Set up CSS transitions using Keyframes instead of AnimationProperty objects

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1260983
(Assignee)

Comment 2

3 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/
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 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+
Attachment #8737006 - Flags: review?(cam) → review+
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

3 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.
Blocks: 1235002

Comment 9

3 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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.