784466 - [css3-animations] we should drop declarations in keyframe rules that have !important

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)]

Attachment: 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-4, no longer working on Mozilla)]

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)]

Attachment: 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)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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)]

(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)]

Attachment: 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-4, no longer working on Mozilla)]

Comment on attachment 662137 [details] [diff] [review]
updated patch

r=dbaron

Comment 10

[Emmanuele Bassi (:ebassi)]

adding checkin-needed, as I don't have a level 2 commit bit for mozilla-inbound.

Comment 11

[Daniel Holbert [:dholbert]]

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]]

https://hg.mozilla.org/mozilla-central/rev/71eacb57041d

Comment 13

[Jean-Yves Perrier [:teoli]]

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