Update animation properties in response to style changes

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)

Assignee

Description

3 years ago
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+
Assignee

Comment 7

3 years ago
(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).

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d23ce3108e5c
https://hg.mozilla.org/mozilla-central/rev/357d4ede2971
https://hg.mozilla.org/mozilla-central/rev/b5e79510a9b0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Note that this added the test in the wrong place in the wpt manifest, so now any other manifest update generates spurious changes....
Assignee

Comment 11

3 years ago
(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?
Assignee

Comment 12

3 years ago
(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.

Updated

3 years ago
Depends on: 1284720

Updated

3 years ago
No longer depends on: 1284720
You need to log in before you can comment on or make changes to this bug.