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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({css1})

Trunk
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.)
(Assignee)

Comment 1

9 years ago
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).
(Assignee)

Comment 2

9 years ago
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.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #357503 - Flags: superreview?(bzbarsky)
Attachment #357503 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

9 years ago
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

9 years ago
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.  ;)
Attachment #357503 - Flags: superreview?(bzbarsky)
Attachment #357503 - Flags: superreview+
Attachment #357503 - Flags: review?(bzbarsky)
Attachment #357503 - Flags: review+
(Assignee)

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/1caacdfd3f5f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.