Closed Bug 474135 Opened 13 years ago Closed 13 years ago

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


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






(Reporter: dbaron, Assigned: dbaron)


(Keywords: css1)


(1 file)

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.)
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).
Attached patch patchSplinter Review
Actually, this isn't all that hard to fix the way the spec intends (option (2) in ), so let's just do that.
Assignee: nobody → dbaron
Attachment #357503 - Flags: superreview?(bzbarsky)
Attachment #357503 - Flags: review?(bzbarsky)
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 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.  ;)
Attachment #357503 - Flags: superreview?(bzbarsky)
Attachment #357503 - Flags: superreview+
Attachment #357503 - Flags: review?(bzbarsky)
Attachment #357503 - Flags: review+
Closed: 13 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.