940216 - Rejected shorthand property values (with junk at the end) should produce an error console message (mentioning the shorthand name, at least)

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Open browser console (Ctrl Shift J)
 1. Load  data:text/html,<div style="flex: inherit abc">
     or   data:text/html,<div style="flex: 1 1 0px  abc">
     or   data:text/html,<div style="flex: none abc">

 2. Check parse error messages in browser console.

ACTUAL RESULTS:
Parse error message looks like this:
> Expected ';' to terminate declaration but found 'abc'.  Declaration dropped.
Note that it doesn't mention the property-name, so it's not particularly helpful.

EXPECTED RESULTS:
Parse error message should look more like this (which is what I get if I swap in the shorthand "background" instead of "flex"):
> Expected end of value but found 'abc'.  Error in parsing
> value for 'background'.  Declaration dropped.

NOTE:
The key difference is that ParseBackground calls ExpectEndProperty() when it knows we should be at the end; we don't have any such calls for ParseFlex right now, but we should add them.

NOTE 2: I'm pretty sure the only issue here is the type of error message we report; we still end up rejecting "flex" values with bogus stuff at the end; we just reject them when the parser is looking for a ";" when it's not thinking it needs to bother reporting the property-name.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think use of ExpectEndProperty() is bad style; recursive descent parsing functions should consume what's correct and only report an error if they require something that's not present.  I think what needs fixing here is the error message.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> ACTUAL RESULTS:
> Parse error message looks like this:
> > Expected ';' to terminate declaration but found 'abc'.  Declaration dropped.

Note: the data URIs from comment 0 no longer trigger any parse errors now. Neither does a "background" property that's followed by junk, like e.g.
 data:text/html,<div style="background: lime foo">a

The change in 'background' behavior is presumably from:
  https://hg.mozilla.org/mozilla-central/rev/05cf275996a2#l1.109
which removed the ExpectEndProperty() call from ParseBackground.

--> Generifying this bug to be about shorthands in general, and about getting *some* message (since currently there seems to be none, at least for 'background' and 'flex')