Closed
Bug 1340961
Opened 8 years ago
Closed 8 years ago
stylo: handle missing final keyframe with !important on CSS Animations properly
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
People
(Reporter: boris, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
For this example [1]: @keyframes important2 { from { opacity: 0.5; transform: translate(100px); } to { opacity: 0.2 !important; /* ignored */ transform: translate(50px); } } [1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/style/test/test_animations_omta.html#140-145 If there is an !important rule in @keyframes, we ignore it. However, we don't handle this case properly. While building the AnimationPropertySegment in BuildSegmentsFromValueEntries() for opacity, we create a missing final segment as [0.2, nullptr]. Its mValue with offset 1.0 is a null RawServoAnimationValue, so we cannot do interpolation for that.
Reporter | ||
Updated•8 years ago
|
Blocks: stylo-css-animations
Assignee | ||
Updated•8 years ago
|
Summary: stylo: Need to handle missing final keyframe on CSS Animations properly → stylo: handle missing final keyframe with !important on CSS Animations properly
Assignee | ||
Comment 1•8 years ago
|
||
A problem I (and other guys) have noticed is that we don't check all properties [1] in @keyframes. But this is not the case. [1] https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/components/style/keyframes.rs#l251
Assignee | ||
Comment 2•8 years ago
|
||
Urgh! In this case we inject values for the missing properties in KeyframesStepValue::Declarations block if it's the initial or finial keyframe. https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/ports/geckolib/glue.rs#l1256
Assignee | ||
Comment 3•8 years ago
|
||
we *have to* inject.
Assignee | ||
Comment 4•8 years ago
|
||
OK. This is highly related to bug 1339332. Once bug 1339332 is fixed, gecko does not fill missing keyframes for CSS animations at all. That means a bunch of our automation test cases will be modified, So, stylo should follow it. But a problem is that bug 1339332 depends additive animation (actually missing keyframe handling), but yes, stylo does not have the one.. it will be tough.
Blocks: 1311257
Assignee | ||
Comment 5•8 years ago
|
||
So, we don't need to inject the missing values into keyframes at all, moreover we should drop KeyframesStepValue::ComputedValues case [1] there. [1] https://hg.mozilla.org/mozilla-central/file/16effd5b21ab/servo/ports/geckolib/glue.rs#l1236
Comment 6•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > OK. This is highly related to bug 1339332. Once bug 1339332 is fixed, gecko > does not fill missing keyframes for CSS animations at all. The plan there is to fill in the missing keyframes, but it simply sets the property values to null. So @keyframes abc { to { opacity: 1 } } Becomes: [ { offset: 0, opacity: null, easing: 'ease' }, { offset: 1, opacity 1 } ] It's because of the easing that we can't just omit the keyframe. (When you call getKeyframes() from Javascript, however, we will resolve the base style and replace 'null' with that value.)
Assignee | ||
Comment 7•8 years ago
|
||
OK. Thanks. That's really good to know. I misunderstood that bug. Then in this bug, we do *inject* mProperty and mServoDeclarationBlock with computed values, and later we will set mServoDeclarationBlock to null there once we fix bug 1311257.
Assignee | ||
Comment 8•8 years ago
|
||
This will partially conflict with a patch for bug 1338927.
Assignee | ||
Comment 9•8 years ago
|
||
The patch does not cover all of missing value case. One example: @keyframes anim { from { opacity: 0; transform: none; } to { transform: none; } } For this case we should fix get_animated_properties as I commented in comment 1.
Assignee | ||
Comment 10•8 years ago
|
||
The servo side fix is something like this. I will send a PR with tests to servo.
Assignee | ||
Updated•8 years ago
|
Attachment #8839293 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8839297 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc797275e38d18925f87d79518fc0a49376c19f9 I've decided to post both of patches in this bug.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
Emilio, could you please take a look these patches? We have to handle following keyframes. @keyframes no-opacity-in-final-keyframe { from { opacity: 0; transform: none; } to { transform: none; } } @keyframes no-opacity-in-initial-keyframe { from { transform: none; } to { opacity: 0; transform: none; } } Thanks!
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8839328 [details] Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes. https://reviewboard.mozilla.org/r/113998/#review115552 ::: servo/ports/geckolib/glue.rs:1492 (Diff revision 1) > .iter() > .filter(|&&(ref declaration, _)| { > declaration.is_animatable() > }); > + > + let mut properties_on_this_keyframe: HashSet<TransitionProperty> = HashSet::new(); We could try to use `PropertyBitfield` for these. ::: servo/ports/geckolib/glue.rs:1514 (Diff revision 1) > + } > + > + // Append missing property values in the initial or the finial keyframes. > + if step.start_percentage.0 == 0. || > + step.start_percentage.0 == 1. { > + for (index, property) in animation.properties_changed.iter().enumerate() { I'm unsure how this works? `properties_changed` only looks at the first keyframe, right? Shouldn't we fix that too?
Attachment #8839328 -
Flags: review?(emilio+bugs)
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8839329 [details] Bug 1340961 - Part 2: Get all animated properties in *all* keyframes. https://reviewboard.mozilla.org/r/114000/#review115554 Ok, should've checked this patch too. r=me with the patches either squashed or reordered, and using `PropertyBitfield`. We can add helpers to either `PropertyBitfield` or `TransitionProperty` to make it easier. Thanks! ::: servo/components/style/keyframes.rs:248 (Diff revision 1) > /// The properties that change in this animation. > pub properties_changed: Vec<TransitionProperty>, > } > > -/// Get all the animated properties in a keyframes animation. Note that it's not > -/// defined what happens when a property is not on a keyframe, so we only peek > +/// Get all the animated properties in a keyframes animation. > +fn get_animated_properties(keyframes: &[Arc<RwLock<Keyframe>>]) -> Vec<TransitionProperty> { Oh, of course this was the second patch, heh :) ::: servo/components/style/keyframes.rs:258 (Diff revision 1) > + let keyframe = keyframe.read(); > - for &(ref declaration, importance) in keyframe.block.read().declarations.iter() { > + for &(ref declaration, importance) in keyframe.block.read().declarations.iter() { > - assert!(!importance.important()); > + assert!(!importance.important()); > > - if let Some(property) = TransitionProperty::from_declaration(declaration) { > + if let Some(property) = TransitionProperty::from_declaration(declaration) { > + if !ret.contains(&property) { This `contains` check is linear, we should use a `PropertyBitfield` ideally, so it's faster.
Attachment #8839329 -
Flags: review?(emilio+bugs) → review+
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8839328 [details] Bug 1340961 - Part 3: Fill computed property values where the propery is missing in initial and final keyframes. https://reviewboard.mozilla.org/r/113998/#review115556 r=me with the comments in the other patch addressed, thanks for doing this!
Attachment #8839328 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
Thank you Emilio, I will add a patch to add a helper for PropertyBitFiled as a part 1 patch and revise order of the rest of patches. Thanks!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8839752 [details] Bug 1340961 - Part 1: Make PropertyFiledBit usable for TransitionProperty. https://reviewboard.mozilla.org/r/114320/#review115856 Nice! thanks for doing this. r=me with the following nits :) ::: servo/components/style/properties/properties.mako.rs:191 (Diff revision 1) > use logical_geometry::WritingMode; > + use properties::animated_properties::TransitionProperty; > > /// A bitfield for all longhand properties, in order to quickly test whether > /// we've seen one of them. > + /// Note: The last bit is for TransitionProperty::All. Do we need this bit? As far as I know we can't get `TransitionProperty::All` with keyframes. I think `All` is enough of a special case that is not worth to reserve a bit for it (I can't think of a use case where it would be useful). Let's use `unreachable!("Tried to {get,set} TransitionProperty::All in a PropertyBitfield")` in the relevant branches instead, and note in the documentation for `get_transition_property` and `has_transition_property` that this method panics if `TransitionProperty::All` is given to them. ::: servo/components/style/properties/properties.mako.rs:246 (Diff revision 1) > } > % endif > % endfor > + > + /// Set the corresponding bit of TransitionProperty. > + pub fn set_property(&mut self, property: &TransitionProperty) { I'd just rename these to `set_transition_property` and `has_transition_property`, since I'd expect `{set,has}_property` to operate on `LonghandId`s.
Attachment #8839752 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22) > ::: servo/components/style/properties/properties.mako.rs:191 > (Diff revision 1) > > use logical_geometry::WritingMode; > > + use properties::animated_properties::TransitionProperty; > > > > /// A bitfield for all longhand properties, in order to quickly test whether > > /// we've seen one of them. > > + /// Note: The last bit is for TransitionProperty::All. > > Do we need this bit? As far as I know we can't get `TransitionProperty::All` > with keyframes. > > I think `All` is enough of a special case that is not worth to reserve a bit > for it (I can't think of a use case where it would be useful). That's right. I was actually thinking the All case should be panic!. > Let's use `unreachable!("Tried to {get,set} TransitionProperty::All in a > PropertyBitfield")` in the relevant branches instead, and note in the > documentation for `get_transition_property` and `has_transition_property` > that this method panics if `TransitionProperty::All` is given to them. I did not know unreachable!. Thanks! > ::: servo/components/style/properties/properties.mako.rs:246 > (Diff revision 1) > > } > > % endif > > % endfor > > + > > + /// Set the corresponding bit of TransitionProperty. > > + pub fn set_property(&mut self, property: &TransitionProperty) { > > I'd just rename these to `set_transition_property` and > `has_transition_property`, since I'd expect `{set,has}_property` to operate > on `LonghandId`s. That's sounds a good idea. Oops. I just renamed it. Unfortunately we have already set_transition_property() for short hand of 'transition'...
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23) > > I'd just rename these to `set_transition_property` and > > `has_transition_property`, since I'd expect `{set,has}_property` to operate > > on `LonghandId`s. > > That's sounds a good idea. Oops. I just renamed it. Unfortunately we have > already set_transition_property() for short hand of 'transition'... Wrong. This was not a short hand. transition-property actually.
Assignee | ||
Comment 25•8 years ago
|
||
I've added _bit suffix to the function names.
Assignee | ||
Comment 26•8 years ago
|
||
Servo PR. https://github.com/servo/servo/pull/15683 We don't need to land any patches in autoland in this bug.
Assignee | ||
Comment 27•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/887ae49f0e2f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•