Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CSS parser syntax error message #1756

Closed
mgol opened this Issue Oct 21, 2014 · 15 comments

Comments

Projects
None yet
5 participants
@mgol
Copy link
Member

commented Oct 21, 2014

Originally reported by walde.christian@… at: http://bugs.jquery.com/ticket/14511

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)

Issue reported for jQuery 1.10.2

@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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. :)

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: timmywil

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

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: timmywil

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

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: walde.christian@…

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. :)

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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.

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: 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

@mgol

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2014

Comment author: dmethvin

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.

@wchristian

This comment has been minimized.

Copy link

commented Oct 21, 2014

Just to leave a note: I still stand by my last comment here.

@dmethvin

This comment has been minimized.

Copy link
Member

commented May 17, 2016

Is this going to happen in 3.0? If so we need to do it and update the upgrade guide. Otherwise I think we should close this ticket and put it on the roadmap. There was some good discussion in jquery/sizzle#326.

@timmywil

This comment has been minimized.

Copy link
Member

commented May 17, 2016

It was pushed to 3.1. This is a one-time code change with a clear end. Once an update lands in Sizzle, this ticket should be closed when we update Sizzle.

@dmethvin dmethvin modified the milestones: 4.0.0, 3.2.0 Sep 26, 2016

@mgol mgol modified the milestones: 4.0.0, 3.4.0 Apr 8, 2019

mgol added a commit to mgol/jquery that referenced this issue Apr 9, 2019

@mgol mgol referenced this issue Apr 9, 2019

Merged

Build: Update Sizzle from 2.3.3 to 2.3.4 #4345

2 of 2 tasks complete

@mgol mgol closed this in #4345 Apr 9, 2019

mgol added a commit that referenced this issue Apr 9, 2019

mgol added a commit that referenced this issue Apr 9, 2019

Build: Update Sizzle from 2.3.3 to 2.3.4
(cherry-picked from 0b2c36a)

Fixes gh-1756
Fixes gh-4170
Fixes gh-4249
Closes gh-4345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.