Closed
Bug 784466
Opened 11 years ago
Closed 11 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•11 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•11 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•11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Assignee: nobody → ebassi
Reporter | ||
Comment 9•11 years ago
|
||
Comment on attachment 662137 [details] [diff] [review] updated patch r=dbaron
Attachment #662137 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 10•11 years ago
|
||
adding checkin-needed, as I don't have a level 2 commit bit for mozilla-inbound.
Keywords: checkin-needed
Comment 11•11 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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71eacb57041d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•11 years ago
|
Keywords: dev-doc-needed
Updated•10 years ago
|
Whiteboard: [DocArea=CSS]
Comment 13•9 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
•