Bug Tracker

Opened 6 years ago

Closed 5 years ago

#14511 closed bug (migrated)

Update CSS parser syntax error message

Reported by: walde.christian@… Owned by: timmywil
Priority: low Milestone: 1.12/2.2
Component: selector Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:

Description

The code for throwing syntax errors in the CSS parser looks as follows:

throw Error("Syntax error, unrecognized expression: "+e)

In practice, when triggered deep inside some third party javascript that generates CSS selectors programmatically, this results in mildly helpful errors like this:

https://dl.dropboxusercontent.com/u/10190786/css_error.png

The only helpful part of this error message is that it actually contains the CSS selector used.

On the confusing side, it looks like an error that might've come from the JS engine of firefox itself, does not mention at all that it's upset about an invalid CSS selector, contains no stacktrace, can't be breakpointed on and when trying to approach it with firebug will be thrown often when trying to step into code that's many layers earlier in the call stack.

A simple change of the error message would make it much more helpful and less confusing:

throw Error("CSS selector syntax error, unrecognized expression: "+e)

Change History (15)

comment:1 Changed 6 years ago by dmethvin

I'm sympathetic, but a Google search usually helps. I'm not sure the additional words there add much to it, and any attempt to add "Sizzle" or "jQuery" would lead people to this very bug tracker rather than to some place like StackOverflow which is where they should go when they've messed up a selector.

comment:2 Changed 6 years ago by walde.christian@…

a Google search usually helps

The usability here is similar to having a numerical error code to google. Unless one has seen the message before and know what it means, one has little chance to figure out that it's talking about a CSS selector.

Excellent error message inform the user as to exactly what is wrong.

The current one merely informs the user that something is wrong.

--

I'm not sure the additional words there add much to it, and any attempt to add "Sizzle" or "jQuery"

If you look at my ticket again i never asked for those terms to be added, but simply the term "CSS selector", which is specifically what the error message is about.

In an optimal world the error would also come with a stack tract to point out where in the code it came from, and the selector parser would, in a case of parse-fail, reparse in order to be able to provide an explanation of what it expected to see, but i'm aware those are non-trivial efforts. However making the error at least clear enough so it can be easily seen what it is about, is very trivial. :)

comment:3 Changed 6 years ago by timmywil

We could change "expression" to "selector", but it seems pretty clear to me as the selector itself is included in the error message.

comment:4 Changed 6 years ago by walde.christian@…

it seems pretty clear to me as the selector itself is included in the error message

Please take a look at my original description again. When this error is triggered in third-party software, including the selector does little to enlighten the developer as to its type, as the developer might not even be aware of classes or ids used by said third-party software.

comment:5 Changed 6 years ago by timmywil

Owner: set to walde.christian@…
Status: newpending

How about something concise like 'Error: Selector Syntax: .flag' ?

comment:6 Changed 6 years ago by walde.christian@…

Status: pendingnew

I'm fine with shortening it, but for maximum benevolence towards future users i'd like to see explicit mention of CSS remain in the message. :)

comment:7 Changed 6 years ago by timmywil

I thought about that, but then I remembered that there are still some users out there who use XPath converters, in which case the selector was not originally CSS. I think it's clear enough without it though.

comment:8 Changed 6 years ago by dmethvin

I'm okay with the suggestion in comment 5, with minor formatting changes:

Error in selector syntax: .flag

We'll need to announce this prominently so that when people use a Google search to find it they'll know what it means and what to do.

Last edited 6 years ago by dmethvin (previous) (diff)

comment:9 Changed 6 years ago by walde.christian@…

This is merely an observation:

If you are concerned that people will not understand what an error message means and will need to google it, then the chance that it is not a very good error message is rather high. :)

Excellent error messages explain what went wrong and offer resolution suggestions.

comment:10 Changed 6 years ago by timmywil

@walde.christian@…

Firstly, developers will google error messages no matter how clear they are. Secondly, resolution suggestions would require a certain psychic ability. There is no way to know what the user is trying to select. The most helpful message simply points out that the selector is invalid and needs to be changed to something that won't throw an error. We can do that in a succinct manner.

@dmethvin: The "Error: " part of the message is automatically inserted by the browser.

Last edited 6 years ago by timmywil (previous) (diff)

comment:11 Changed 6 years ago by walde.christian@…

@timmywil

I respectfully disagree on both points in the general case, due to experience with many developers of many experience levels and due to experience with writing parsers; however i can see that the scope of, constraint of and experiences with jQuery would make you hold your positions and don't think arguing these points is necessary for the main point of this ticket. :)

That said, after having reread and thought about it, i would like to say that i see the above shortened proposal as both better than the original and below the helpfulness it could reasonably have, given the context of this project.

The average user is not helped by seeing an error message that has been golfed for maximal brevity. You mention xpath converters, which i am unfamilar with, but assume are a piece of code that takes xpath entered by a user and converts it into a CSS selector to be used internally. If that is the case and the xpath converter is a once-sanctioned part of jQuery, then i'd recommend explicitly acknowledging that, possibly in such a way:

Error: Invalid selector syntax (check original CSS/XPath expression): .flag

comment:12 Changed 6 years ago by dmethvin

Milestone: None1.12/2.2
Status: newopen

I'll mark this for 1.12/2.2; before we ship, the title of the ticket should change to something useful for the changelog and Google-ability.

comment:13 Changed 6 years ago by timmywil

Component: unfiledselector
Priority: undecidedlow
Summary: CSS parser syntax error message only very mildly helpfulUpdate CSS parser syntax error message

comment:14 Changed 6 years ago by timmywil

Owner: changed from walde.christian@… to timmywil
Status: openassigned

comment:15 Changed 5 years ago by m_gol

Resolution: migrated
Status: assignedclosed
Note: See TracTickets for help on using tickets.