474135 - handling of 'none' values inside 'list-style' shorthand is broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

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

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/1caacdfd3f5f