Closed
Bug 784466
Opened 13 years ago
Closed 13 years ago
[css3-animations] we should drop declarations in keyframe rules that have !important
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: dbaron, Assigned: ebassi)
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])
Attachments
(1 file, 2 obsolete files)
11.20 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
Per a CSS working group resolution sometime in the last few months, http://dev.w3.org/csswg/css3-animations/#keyframes now says:
In addition, keyframe rule declarations qualified with !important are ignored.
We should implement this. Doing so means:
* reporting them as a parse error
* removing the code that we have to make them do something
so it should be roughly neutral on code size, I think. Each change should just be a few lines.
Assignee | ||
Comment 1•13 years ago
|
||
this is an initial, trivial patch, that just removes the mImportantData serialization.
as for CSS parser error, I have a more complex patch that adds a "check important" to ParseDeclaration and ParseDeclarationBlock that will raise a parse error if a declaration inside a keyframe rule declaration is marked with !important, but I wanted a clarification as to what to do with the actual declaration in case of error: should add it to the block, or drop it?
Attachment #659680 -
Flags: feedback?(dbaron)
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 659680 [details] [diff] [review]
Initial trivial patch
> /* virtual */ void
> nsCSSKeyframeRule::MapRuleInfoInto(nsRuleData* aRuleData)
> {
> // We need to implement MapRuleInfoInto because the animation manager
> // constructs a rule node pointing to us in order to compute the
> // styles it needs to animate.
>
>- // FIXME (spec): The spec doesn't say what to do with !important.
>- // We'll just map them.
>- if (mDeclaration->HasImportantData()) {
>- mDeclaration->MapImportantRuleInfoInto(aRuleData);
>- }
>+ // The spec says that !important declarations should just be ignored
> mDeclaration->MapNormalRuleInfoInto(aRuleData);
I think it's worth asserting that !mDeclaration->HasImportantData()
>+ @keyframes important2 {
>+ from { margin-top: 0px; margin-bottom: 0px; }
>+ to { margin-top: 150px !important; /* ignored */
>+ margin-bottom: 50px; }
>+ }
This set seems slightly more interesting if margin-top at 0% is nonzero.
(In reply to Emmanuele Bassi (:ebassi) from comment #1)
> as for CSS parser error, I have a more complex patch that adds a "check
> important" to ParseDeclaration and ParseDeclarationBlock that will raise a
> parse error if a declaration inside a keyframe rule declaration is marked
> with !important, but I wanted a clarification as to what to do with the
> actual declaration in case of error: should add it to the block, or drop it?
Drop it. (That's what the "are ignored" in the spec quote above means.)
Attachment #659680 -
Flags: feedback?(dbaron) → feedback+
Assignee | ||
Comment 3•13 years ago
|
||
full patch that drops the declarations marked as !important.
the test suite highlights an issue with the parser for which I'll need some further guidance:
- in the important1 test case the '50%' keyframe has an empty block (one declaration is ignored) so we keep the frame but the property is assumed to be the default, instead of just ignoring the whole keyframe; what is the expected behaviour?
- in the important2 test case the 'to' keyframe seems to be ignoring the margin-bottom as well, so there could be some issue in skipping the ignored declaration - but SkipDeclration() should still behave correctly even with an !important token - so I'm a bit stumped on that.
Attachment #659680 -
Attachment is obsolete: true
Attachment #661182 -
Flags: review?(dbaron)
Assignee | ||
Comment 4•13 years ago
|
||
test result on TBPL: https://tbpl.mozilla.org/php/getParsedLog.php?id=15216266&tree=Try
18766 INFO TEST-PASS | /tests/layout/style/test/test_animations.html | important1 test at 0s - 50px should equal 50px
18767 INFO TEST-PASS | /tests/layout/style/test/test_animations.html | important1 test at 0.5s - 75px should equal 75px
18768 INFO TEST-PASS | /tests/layout/style/test/test_animations.html | important1 test at 1s - 100px should equal 100px
18769 INFO TEST-PASS | /tests/layout/style/test/test_animations.html | important2 (margin-top) test at 0s - 50px should equal 50px
18770 INFO TEST-PASS | /tests/layout/style/test/test_animations.html | important2 (margin-bottom) test at 0s - 100px should equal 100px
18771 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_animations.html | important2 (margin-top) test at 1s - got 0px, expected 50px
18772 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_animations.html | important2 (margin-bottom) test at 1s - got 0px, expected 50px
Reporter | ||
Comment 5•13 years ago
|
||
Comment on attachment 661182 [details] [diff] [review]
full patch, updated after feedback
I'll start with the tests:
>+new_div("animation: important1 1s linear forwards");
>+is(cs.marginTop, "50px", "important1 test at 0s");
>+advance_clock(500);
>+is(cs.marginTop, "75px", "important1 test at 0.5s");
>+advance_clock(1000);
>+is(cs.marginTop, "100px", "important1 test at 1s");
>+done_div();
This test looks correct, except the "at 1s" is really "at 1.5s"; still a valid test, though.
>+new_div("animation: important2 1s linear forwards");
>+is(cs.marginTop, "50px", "important2 (margin-top) test at 0s");
>+is(cs.marginBottom, "100px", "important2 (margin-bottom) test at 0s");
>+advance_clock(1000);
>+is(cs.marginTop, "50px", "important2 (margin-top) test at 1s");
>+is(cs.marginBottom, "50px", "important2 (margin-bottom) test at 1s");
>+done_div();
This test seems wrong; the initial value of margin-top is 0px, so the marginTop test at 1s should be checking for 0px.
But the marginBottom test *should* be checking for 50px, which suggests you have a parsing bug.
Reporter | ||
Comment 6•13 years ago
|
||
Comment on attachment 661182 [details] [diff] [review]
full patch, updated after feedback
So I don't actually see what's wrong on the code side that would cause the unexpected behavior. However, a few notes:
* I think the approach you're taking in ParseDeclaration looks like it should work, but I think there's also a simpler way: check the flag for whether to look for important, and only call ParsePriority if it's set. (If it's not, set status to ePriority_None.)
* You use a PEBadImportant error string, but it's not introduced in the patch. However, I'm not sure it's needed (see previous comment).
* I'm also not crazy about |haveBraces| not being a boolean. Maybe keep it a boolean, and turn it into bits lower, at the call site of ParseDeclarationBlock.
* I'm also not crazy about the eDecl_Check names. Perhaps aFlags instead of aChecksMask, and call the flags eParseDeclaration_InBraces and eParseDeclaration_AllowImportant.
* I also don't like bitfield constants for 0, since people tend to try |-ing them with other constants. Can you just use 0 instead of eDeclCheck_None.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to David Baron [:dbaron] from comment #5)
> Comment on attachment 661182 [details] [diff] [review]
> full patch, updated after feedback
>
> I'll start with the tests:
>
> >+new_div("animation: important1 1s linear forwards");
> >+is(cs.marginTop, "50px", "important1 test at 0s");
> >+advance_clock(500);
> >+is(cs.marginTop, "75px", "important1 test at 0.5s");
> >+advance_clock(1000);
> >+is(cs.marginTop, "100px", "important1 test at 1s");
> >+done_div();
>
> This test looks correct, except the "at 1s" is really "at 1.5s"; still a
> valid test, though.
>
> >+new_div("animation: important2 1s linear forwards");
> >+is(cs.marginTop, "50px", "important2 (margin-top) test at 0s");
> >+is(cs.marginBottom, "100px", "important2 (margin-bottom) test at 0s");
> >+advance_clock(1000);
> >+is(cs.marginTop, "50px", "important2 (margin-top) test at 1s");
> >+is(cs.marginBottom, "50px", "important2 (margin-bottom) test at 1s");
> >+done_div();
>
> This test seems wrong; the initial value of margin-top is 0px, so the
> marginTop test at 1s should be checking for 0px.
the initial value is set at 50px in the important2 keyframe from rule, but the final value is ignored, so I think it should be the inherited value - i.e. 0px.
> But the marginBottom test *should* be checking for 50px, which suggests you
> have a parsing bug.
indeed.
Assignee | ||
Comment 8•13 years ago
|
||
updated after review:
- renamed eDeclCheck_* with eParseDeclaration_*;
- removed the None enumeration value;
- flipped the Important flag to be AllowImportant, and always be set except in the ParseDeclarationBlock() for @keyframe rules;
- kept |haveBraces| as a boolean;
- updated the tests.
try server says it's green.
Attachment #661182 -
Attachment is obsolete: true
Attachment #661182 -
Flags: review?(dbaron)
Attachment #662137 -
Flags: review?(dbaron)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → ebassi
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 662137 [details] [diff] [review]
updated patch
r=dbaron
Attachment #662137 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 10•13 years ago
|
||
adding checkin-needed, as I don't have a level 2 commit bit for mozilla-inbound.
Keywords: checkin-needed
Comment 11•13 years ago
|
||
Dropped "we should" from commit message (to describe the change, rather than the issue), and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/71eacb57041d
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Comment 12•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: dev-doc-needed
Updated•11 years ago
|
Whiteboard: [DocArea=CSS]
Comment 13•10 years ago
|
||
Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/@keyframes#!important_in_a_keyframe
and
https://developer.mozilla.org/en-US/Firefox/Releases/19#CSS
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•