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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: dbaron, Assigned: ebassi)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Initial trivial patch (obsolete) — Splinter Review
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)
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+
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)
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 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 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.
(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.
Attached patch updated patchSplinter Review
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: nobody → ebassi
Comment on attachment 662137 [details] [diff] [review] updated patch r=dbaron
Attachment #662137 - Flags: review?(dbaron) → review+
adding checkin-needed, as I don't have a level 2 commit bit for mozilla-inbound.
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: dev-doc-needed
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: