Closed Bug 1260983 Opened 4 years ago Closed 4 years ago

Update animation properties in response to style changes

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.
Attachment #8737036 - Flags: review?(cam) → review+
Comment on attachment 8737036 [details]
MozReview Request: Bug 1260983 - Update animation properties when the style context changes; r?heycam

https://reviewboard.mozilla.org/r/43671/#review41931
Comment on attachment 8737037 [details]
MozReview Request: Bug 1260983 - Allow creating animations with a target element not bound to a document; r?heycam

https://reviewboard.mozilla.org/r/43673/#review41933
Attachment #8737037 - Flags: review?(cam) → review+
Comment on attachment 8737038 [details]
MozReview Request: Bug 1260983 - Add tests for changes to effect value context; r?heycam

https://reviewboard.mozilla.org/r/43675/#review41935

::: testing/web-platform/meta/web-animations/animation-model/keyframes/effect-value-context.html.ini:4
(Diff revision 1)
> +    expected: FAIL
> +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1254424

What's missing for this test to pass?  (Why are the changes in this bug insufficient?)
Attachment #8737038 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #6)
> Comment on attachment 8737038 [details]
> MozReview Request: Bug 1260983 - Add tests for changes to effect value
> context; r?heycam
> 
> https://reviewboard.mozilla.org/r/43675/#review41935
> 
> :::
> testing/web-platform/meta/web-animations/animation-model/keyframes/effect-
> value-context.html.ini:4
> (Diff revision 1)
> > +    expected: FAIL
> > +    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1254424
> 
> What's missing for this test to pass?  (Why are the changes in this bug
> insufficient?)

As I understand it, in this bug we're just detecting changes to the style context on the element, but not changes to the style context on ancestors. I think I worked out that we can fix that by calling UpdateEffectProperties earlier in nsStyleSet::GetContext[1] but I was concerned about the performance implications of calling that too much.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8729365&action=diff#a/layout/style/nsStyleSet.cpp_sec2

So I thought I'd look into that in a separate bug since this is an existing issue with CSS animations. In that bug we can try to find a more efficient means of reflecting changes to font-size etc (e.g. only calling UpdateEffectProperties when we know it is a change to font-size and we are using context-sensitive property values).
Note that this added the test in the wrong place in the wpt manifest, so now any other manifest update generates spurious changes....
(In reply to Boris Zbarsky [:bz] from comment #10)
> Note that this added the test in the wrong place in the wpt manifest, so now
> any other manifest update generates spurious changes....

What's the issue?
(In reply to Brian Birtles (:birtles) from comment #11)
> (In reply to Boris Zbarsky [:bz] from comment #10)
> > Note that this added the test in the wrong place in the wpt manifest, so now
> > any other manifest update generates spurious changes....
> 
> What's the issue?

Oh I see. I'm not sure what happened with the merge there. Bug 1260572 is the real culprit.
Depends on: 1284720
No longer depends on: 1284720
You need to log in before you can comment on or make changes to this bug.