Closed Bug 1260976 Opened 6 years ago Closed 6 years ago

Set up CSS transitions using Keyframes instead of AnimationProperty objects

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(3 files)

No description provided.
Blocks: 1260983
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. :-)
(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
You need to log in before you can comment on or make changes to this bug.