Closed Bug 940778 Opened 11 years ago Closed 11 years ago

counter-reset & counter-increment properties incorrectly accept "inherit", "initial", and "none" appended to other values

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: dholbert, Assigned: jtd)

References

Details

(Whiteboard: [good first verify])

Attachments

(2 files, 2 obsolete files)

testcase 1
712 bytes, text/html
Details
patch, check idents for none and css-wide keywords
3.41 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
STR:
1. Open browser console.
2. Load this URL:
 data:text/html,<!DOCTYPE html><div style="counter-reset: foo inherit">
   ...or:
 data:text/html,<!DOCTYPE html><div style="counter-increment: foo inherit">
   
EXPECTED RESULTS: There should be a parse error in browser console for "inherit" being included as part of a larger value.

ACTUAL RESULTS: No parse error.
[Side note: if you use the special value "none", *then* we (correctly) reject "inherit". That is to say, this URL correctly produces a parse error...
 data:text/html,<!DOCTYPE html><div style="counter-reset: none inherit">
...though it's somewhat unhelpful as it doesn't mention the property-name. It's the same as the "actual results" in bug 940216. We probably need an ExpectEndProperty() inserted somewhere to address that. Depending on how much rewriting this bug involves, it could make sense to address that here, or maybe in its own separate bug.]
Spec reference:
> Value:  	[ <identifier> <integer>? ]+ | none | inherit
[...]
> The keywords 'none', 'inherit' and 'initial' must not be used as counter names.
> A value of 'none' on its own means no counters are reset, resp. incremented.
> 'Inherit' on its own has its usual meaning (see 6.2.1). 'Initial' is reserved
> for future use.
http://www.w3.org/TR/CSS21/generate.html#propdef-counter-increment

Note that we also incorrectly accept "none" as a suffix here, too -- i.e. this doesn't produce a parse error (but it should):
 data:text/html,<!DOCTYPE html><div style="counter-reset: foo none">

It looks like we're incorrectly treating the final token as a counter-name, despite the spec's prohibition on that for these specific keywords.
Summary: counter-reset & counter-increment properties incorrectly accept "inherit" appended to other values. → counter-reset & counter-increment properties incorrectly accept "inherit", "initial", and "none" appended to other values
Attached file testcase 1 — 
Here's a testcase, demonstrating that we do indeed honor "inherit" and "none" as counter names, at least if they're not first in the list.  (I have "foo 1" as a dummy unused counter-name in this testcase, to get the parser in gear for list-of-counters handling when it hits the forbidden token.)

The behavior here is strangely interoperable -- Chrome dev channel, Firefox Nightly, and Opera 12.16 all agree on the (wrong) rendering here.

Also: If I take out "foo 1" dummy counter (leaving us with e.g. "counter-increment: inherit 5;"), *then* Chrome and Firefox get it right (they stop treating "inherit" as a counter-name), but Opera keeps the broken rendering.
Check for keywords disallowed in user idents
Attachment #8334992 - Flags: review?(dholbert)
Comment on attachment 8334992 [details] [diff] [review]
patch, check idents for inherit/initial/default

I want to get clarification from the csswg first, since (in retrospect) the spec text "must not be used as counter names" does not technically mean "are invalid counter names". (though that probably is what was intended.
OK, dbaron says in IRC that these really should just be invalid counter-names, so I think we're fine going ahead with fixing it.

Per IRC, "none" should also be in your list of prohibited keywords.

"unset" should be there as well. I think you really want to prohibit all of the CSS-wide keywords ( http://www.w3.org/TR/css3-values/#common-keywords ), plus "none".
Revised to include 'none' and 'unset' in list of disallowed idents.  Also, let bug 940229 deal with 'foo inherit' and 'foo initial' cases.
Attachment #8334992 - Attachment is obsolete: true
Attachment #8334992 - Flags: review?(dholbert)
Attachment #8335004 - Flags: review?(dholbert)
Comment on attachment 8335004 [details] [diff] [review]
patch, check idents for inherit/initial/default

># HG changeset patch
># Parent 0a87f31b86f1b9243d291a98abda5180320894af
># User John Daggett <jdaggett@mozilla.com>
>Bug 940778 - counter properties allow inherit/initial/default as counter names.
>
>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp
>@@ -8520,16 +8520,25 @@ CSSParserImpl::ParseCounterData(nsCSSPro
>   nsCSSValue value;
>   if (!ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>     if (!GetToken(true) || mToken.mType != eCSSToken_Ident) {
>       return false;
>     }
> 
>     nsCSSValuePairList *cur = value.SetPairListValue();
>     for (;;) {
>+      // check for keywords 'inherit', 'default', 'initial'
>+      nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
>+      if (keyword == eCSSKeyword_inherit ||
>+          keyword == eCSSKeyword_default ||
>+          keyword == eCSSKeyword_none ||
>+          keyword == eCSSKeyword_unset ||
>+          keyword == eCSSKeyword_initial) {
>+        return false;
>+      }
>       cur->mXValue.SetStringValue(mToken.mIdent, eCSSUnit_Ident);

The comment doesn't quite match the list there. Maybe replace comment with something like:
 // check for "none" and the CSS-wide keywords, which can't be used as counter names

Also: We probably should only do the LookupKeyword if mToken.mType == eCSSToken_Ident, right? (Perhaps we really should be bailing out entirely if mToken.mType isn't eCSSToken_Ident ?)

>+++ b/layout/style/test/property_database.js
>-		invalid_values: []
>+		invalid_values: [ "none initial", "foo inherit", "foo 3 inherit", "foo initial", "inherit foo", "foo default" ]

Per IRC, the "inherit" variants should be covered by bug 940229 (but we do need the "none" variants here, since "none" doesn't have the same universal interpretation).

>+++ b/layout/style/test/property_database.js
>-		invalid_values: []
>+		invalid_values: [ "foo none", "foo 3 none" ]

Maybe add "none foo", "none foo 3" for good measure (for both properties).
(In reply to Daniel Holbert [:dholbert] from comment #8)
> Also: We probably should only do the LookupKeyword if mToken.mType ==
> eCSSToken_Ident, right? (Perhaps we really should be bailing out entirely if
> mToken.mType isn't eCSSToken_Ident ?)

Oh, I guess we sort of check that at the end of the loop:
> 8540       if (!GetToken(true) || mToken.mType != eCSSToken_Ident) {

So never mind about that part.
Comment on attachment 8335004 [details] [diff] [review]
patch, check idents for inherit/initial/default

Yeah, and we check before the loop, too.

OK, so this is r=me with Comment 8 addressed, skipping the "Also: We probably should" part (about ident), and with a better commit message than this:
>Bug 940778 - counter properties allow inherit/initial/default as counter names.
:)
Attachment #8335004 - Flags: review?(dholbert) → review+
Blocks: 940229
Updated based on review comments.
Attachment #8335004 - Attachment is obsolete: true
Attachment #8335024 - Flags: review?(dholbert)
Comment on attachment 8335024 [details] [diff] [review]
patch, check idents for none and css-wide keywords

ah, midaired. :)

r=me with a better commit message (describing the change instead of the bug)
Attachment #8335024 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #5)
> spec text "must not be used as counter names" does not technically mean "are
> invalid counter names". (though that probably is what was intended.

Following up on this -- I'm pretty sure these properties really want <custom-ident>, not <ident>/<identifier>.

I emailed the csswg to ask about this:
 http://lists.w3.org/Archives/Public/www-style/2013Nov/0353.html
...and Tab already replied, saying <custom-ident> is correct & he's going to update the css-lists spec (where these properties live now). Excellent.
 http://lists.w3.org/Archives/Public/www-style/2013Nov/0354.html
https://hg.mozilla.org/mozilla-central/rev/f30dfa0f184d
Assignee: nobody → jdaggett
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Daniel Holbert [:dholbert] from comment #14)
> ...and Tab already replied, saying <custom-ident> is correct & he's going to
> update the css-lists spec (where these properties live now). Excellent.
>  http://lists.w3.org/Archives/Public/www-style/2013Nov/0354.html

Following up on this -- that spec-change has gone through. Updated ED says:
 # Name: 	counter-reset
 # Value: 	[ <custom-ident> <integer>? ]+ | none
 [...]
 # Name: 	counter-set
 # Value: 	[ <custom-ident> <integer>? ]+ | none
 [...]
 # Name: 	counter-increment
 # Value: 	[ <custom-ident> <integer>? ]+ | none

http://dev.w3.org/csswg/css-lists/#counter-properties
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: