Closed
Bug 1260976
Opened 8 years ago
Closed 8 years ago
Set up CSS transitions using Keyframes instead of AnimationProperty objects
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(3 files)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43637/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43637/
Attachment #8737004 -
Flags: review?(cam)
Attachment #8737005 -
Flags: review?(cam)
Attachment #8737006 -
Flags: review?(cam)
Assignee | ||
Comment 2•8 years ago
|
||
Although we know that the animation properties will always be filled in for a transition in the cases where we need to query them (going forward we will have a situation where an animation may only have frames, not properties, but that will only happen when the animation isn't attached to an element or the element is not attached to a document, but we don't run animations in that case and cancel existing ones when we enter that state so although they *can* enter that state, we'll never run these methods on them when they do), we still want to move towards making frames the primary unit for interacting with animation values since frames always exist and represent the public interface. Ultimately it would be good to make the properties array on a KeyframeEffect(ReadOnly) an encapsulated detail so that we can freely change their structure (e.g. segments might not be the best setup, it might be better to just have arrays of free-standing values to avoid the duplication of values when segments are continuous). This patch removes or encapsulates a few references to properties and simplifies the code at the same time. Review commit: https://reviewboard.mozilla.org/r/43639/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43639/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43641/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43641/
Comment 4•8 years ago
|
||
Comment on attachment 8737004 [details] MozReview Request: Bug 1260976 - Make nsTransitionManager use Keyframe objects to set up transitions; r?heycam https://reviewboard.mozilla.org/r/43637/#review41923 ::: layout/style/nsTransitionManager.cpp:748 (Diff revision 1) > + Keyframe& fromFrame = *keyframes.AppendElement(); > + fromFrame.mOffset.emplace(0.0); > + PropertyValuePair& pv = *fromFrame.mPropertyValues.AppendElement(); > + pv.mProperty = aProperty; > + DebugOnly<bool> uncomputeResult = > + StyleAnimationValue::UncomputeValue(aProperty, Move(aStartValue), > + pv.mValue); > + MOZ_ASSERT(uncomputeResult, > + "Unable to get specified value from computed from value"); Consider factoring out this keyframe creation since it's basically duplicated just below (apart from the timing function setting)?
Attachment #8737004 -
Flags: review?(cam) → review+
Comment 5•8 years ago
|
||
Comment on attachment 8737005 [details] MozReview Request: Bug 1260976 - Remove some references to properties within nsTransitionManager; r?heycam https://reviewboard.mozilla.org/r/43639/#review41925
Attachment #8737005 -
Flags: review?(cam) → review+
Updated•8 years ago
|
Attachment #8737006 -
Flags: review?(cam) → review+
Comment 6•8 years ago
|
||
Comment on attachment 8737006 [details] MozReview Request: Bug 1260976 - Remove the old AnimationProperty-based GetFrames; r?heycam https://reviewboard.mozilla.org/r/43641/#review41927 I like seeing code removal, especially when it's code I wrote that's a bit complex. :-)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #4) > ::: layout/style/nsTransitionManager.cpp:748 > (Diff revision 1) > > + Keyframe& fromFrame = *keyframes.AppendElement(); > > + fromFrame.mOffset.emplace(0.0); > > + PropertyValuePair& pv = *fromFrame.mPropertyValues.AppendElement(); > > + pv.mProperty = aProperty; > > + DebugOnly<bool> uncomputeResult = > > + StyleAnimationValue::UncomputeValue(aProperty, Move(aStartValue), > > + pv.mValue); > > + MOZ_ASSERT(uncomputeResult, > > + "Unable to get specified value from computed from value"); > > Consider factoring out this keyframe creation since it's basically > duplicated just below (apart from the timing function setting)? Good idea. Fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bd962907de https://hg.mozilla.org/integration/mozilla-inbound/rev/3791a7ec5d6f https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf416db49d5
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82bd962907de https://hg.mozilla.org/mozilla-central/rev/3791a7ec5d6f https://hg.mozilla.org/mozilla-central/rev/2cf416db49d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•