Last Comment Bug 784466 - [css3-animations] we should drop declarations in keyframe rules that have !important
: [css3-animations] we should drop declarations in keyframe rules that have !im...
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Emmanuele Bassi (:ebassi)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-21 12:35 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2014-12-28 06:48 PST (History)
5 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial trivial patch (3.19 KB, patch)
2012-09-10 05:26 PDT, Emmanuele Bassi (:ebassi)
dbaron: feedback+
Details | Diff | Splinter Review
full patch, updated after feedback (11.22 KB, patch)
2012-09-14 05:02 PDT, Emmanuele Bassi (:ebassi)
no flags Details | Diff | Splinter Review
updated patch (11.20 KB, patch)
2012-09-18 06:30 PDT, Emmanuele Bassi (:ebassi)
dbaron: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-21 12:35:23 PDT
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.
Comment 1 Emmanuele Bassi (:ebassi) 2012-09-10 05:26:53 PDT
Created attachment 659680 [details] [diff] [review]
Initial trivial patch

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?
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-10 16:10:11 PDT
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.)
Comment 3 Emmanuele Bassi (:ebassi) 2012-09-14 05:02:55 PDT
Created attachment 661182 [details] [diff] [review]
full patch, updated after feedback

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.
Comment 4 Emmanuele Bassi (:ebassi) 2012-09-14 08:03:21 PDT
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
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-14 14:28:41 PDT
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.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-14 14:44:26 PDT
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.
Comment 7 Emmanuele Bassi (:ebassi) 2012-09-18 06:25:29 PDT
(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.
Comment 8 Emmanuele Bassi (:ebassi) 2012-09-18 06:30:15 PDT
Created attachment 662137 [details] [diff] [review]
updated patch

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.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-10-09 13:15:29 PDT
Comment on attachment 662137 [details] [diff] [review]
updated patch

r=dbaron
Comment 10 Emmanuele Bassi (:ebassi) 2012-10-16 04:30:10 PDT
adding checkin-needed, as I don't have a level 2 commit bit for mozilla-inbound.
Comment 11 Daniel Holbert [:dholbert] 2012-10-16 09:23:51 PDT
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
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-16 18:58:05 PDT
https://hg.mozilla.org/mozilla-central/rev/71eacb57041d

Note You need to log in before you can comment on or make changes to this bug.