Last Comment Bug 474135 - handling of 'none' values inside 'list-style' shorthand is broken
: handling of 'none' values inside 'list-style' shorthand is broken
: css1
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
P3 normal (vote)
: mozilla1.9.2a1
Assigned To: David Baron :dbaron: ⌚️UTC-8
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2009-01-17 10:34 PST by David Baron :dbaron: ⌚️UTC-8
Modified: 2009-01-22 20:27 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (18.43 KB, patch)
2009-01-17 13:32 PST, David Baron :dbaron: ⌚️UTC-8
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description User image David Baron :dbaron: ⌚️UTC-8 2009-01-17 10:34:45 PST
Our handling of 'none' values inside the 'list-style' shorthand is broken.  This is responsible for the remaining failures in layout/style/test/test_value_storage.html .

However, the spec isn't entirely clear here.  See:
and the testcase:

Our behavior would make considerably more sense than it does now if CSSParserImpl::ParseChoice had a 'break;' statement right after the 'found |= bit' to break out of the inner loop, which would make it try parsing every value using each of the property parsers in the order given (rather than using the property parsers out of order in some cases).  (This explains why 'outside none' behaves differently from 'none outside'; in both cases both values are parsed in a single outer loop in ParseChoice.)

However, hopefully the spec will be clarified soon, and we can implement the correct behavior, whatever that is.  (I think the most correct behavior based on the current spec is probably (2) in , although that's probably also the hardest to implement.)
Comment 1 User image David Baron :dbaron: ⌚️UTC-8 2009-01-17 10:39:14 PST
Note that once we fix this, I think we should put that break in the inner loop in ParseChoice, since it would be slightly more efficient that way (in that currently we can go through a loop and a half without parsing anything before finishing, rather than only a single full loop).
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2009-01-17 13:32:36 PST
Created attachment 357503 [details] [diff] [review]

Actually, this isn't all that hard to fix the way the spec intends (option (2) in ), so let's just do that.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2009-01-22 09:20:32 PST
The basic idea of this patch is that we add an extra subproperty (taking only none, inherit, or initial) for the call to ParseChoice, and put it first, so that the first none always gets assigned to it.  If there are two nones, the second none would get assigned to whichever the next property taking a none is.  Then if we got the 'none', we check that there's room for it for at least one of the subproperties that is none, and we default both of them to none instead of their initial values (although those are the same for list-style-image).
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2009-01-22 11:30:28 PST
Comment on attachment 357503 [details] [diff] [review]

>+++ b/layout/style/nsCSSParser.cpp
>+  for (PRUint32 index = 1; index < NS_ARRAY_LENGTH(listStyleIDs); ++index) {

This could use a comment about wanting to not append the fake value, so starting at 1.

r+sr=bzbarsky.  Nice to see all those todos go away.  ;)
Comment 5 User image David Baron :dbaron: ⌚️UTC-8 2009-01-22 20:27:46 PST

Note You need to log in before you can comment on or make changes to this bug.