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

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

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

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

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

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

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

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

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.