Last Comment Bug 474135 - handling of 'none' values inside 'list-style' shorthand is broken
: handling of 'none' values inside 'list-style' shorthand is broken
Status: RESOLVED FIXED
: 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-7 (busy September 14-25)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-17 10:34 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2009-01-22 20:27 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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:
http://lists.w3.org/Archives/Public/www-style/2008Dec/thread.html#msg156
http://wiki.csswg.org/spec/css2.1#issue-94
and the testcase:
http://lists.w3.org/Archives/Public/www-archive/2008Dec/att-0028/list-style.html


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 http://lists.w3.org/Archives/Public/www-style/2008Dec/0156.html , although that's probably also the hardest to implement.)
Comment 1 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-01-17 13:32:36 PST
Created attachment 357503 [details] [diff] [review]
patch

Actually, this isn't all that hard to fix the way the spec intends (option (2) in http://lists.w3.org/Archives/Public/www-style/2008Dec/0156.html ), so let's just do that.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 Boris Zbarsky [:bz] (still a bit busy) 2009-01-22 11:30:28 PST
Comment on attachment 357503 [details] [diff] [review]
patch

>+++ 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-01-22 20:27:46 PST
http://hg.mozilla.org/mozilla-central/rev/1caacdfd3f5f

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