Skip to main content

Bug Tracker

Side navigation

#14511 closed bug (migrated)

Opened November 04, 2013 03:14PM UTC

Closed October 21, 2014 12:23AM UTC

Update CSS parser syntax error message

Reported by: walde.christian@gmail.com 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:

[[Image(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)
Attachments (0)
Change History (15)

Changed November 10, 2013 07:07PM UTC by dmethvin comment:1

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.

Changed November 11, 2013 09:52AM UTC by walde.christian@gmail.com comment:2

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

Changed November 11, 2013 04:54PM UTC by timmywil comment:3

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

Changed November 13, 2013 02:42AM UTC by walde.christian@gmail.com comment:4

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.

Changed December 06, 2013 05:37PM UTC by timmywil comment:5

owner: → walde.christian@gmail.com
status: newpending

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

Changed December 06, 2013 06:21PM UTC by walde.christian@gmail.com comment:6

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

Changed December 06, 2013 09:08PM UTC by timmywil comment:7

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.

Changed December 08, 2013 03:50PM UTC by dmethvin comment:8

_comment0: 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.1386517893443212

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.

Changed December 09, 2013 09:52AM UTC by walde.christian@gmail.com comment:9

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.

Changed December 09, 2013 02:42PM UTC by timmywil comment:10

_comment0: @walde.christian@gmail.com \ \ Firstly, developers will google error messages no matter how clear it is. 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.1386600207076446
_comment1: @walde.christian@gmail.com \ \ 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.1386600218169694

@walde.christian@gmail.com

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.

Changed December 09, 2013 03:05PM UTC by walde.christian@gmail.com comment:11

@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

Changed January 05, 2014 01:59AM UTC by dmethvin comment:12

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.

Changed January 23, 2014 05:00PM UTC by timmywil comment:13

component: unfiledselector
priority: undecidedlow
summary: CSS parser syntax error message only very mildly helpfulUpdate CSS parser syntax error message

Changed January 23, 2014 05:00PM UTC by timmywil comment:14

owner: walde.christian@gmail.comtimmywil
status: openassigned

Changed October 21, 2014 12:23AM UTC by m_gol comment:15

resolution: → migrated
status: assignedclosed