1260983 - Update animation properties in response to style changes

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: MozReview Request: Bug 1260983 - Update animation properties when the style context changes; r?heycam

Review commit: https://reviewboard.mozilla.org/r/43671/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43671/

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: MozReview Request: Bug 1260983 - Allow creating animations with a target element not bound to a document; r?heycam

Review commit: https://reviewboard.mozilla.org/r/43673/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43673/

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: MozReview Request: Bug 1260983 - Add tests for changes to effect value context; r?heycam

Review commit: https://reviewboard.mozilla.org/r/43675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43675/

Comment 4

[Cameron McCormack (:heycam)]

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 5

[Cameron McCormack (:heycam)]

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

Comment 6

[Cameron McCormack (:heycam)]

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?)

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d23ce3108e5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/357d4ede2971
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e79510a9b0

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d23ce3108e5c
https://hg.mozilla.org/mozilla-central/rev/357d4ede2971
https://hg.mozilla.org/mozilla-central/rev/b5e79510a9b0

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that this added the test in the wrong place in the wpt manifest, so now any other manifest update generates spurious changes....

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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?

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.