Closed
Bug 1430884
Opened 7 years ago
Closed 6 years ago
Throttle transform animations on out of view element that the transform animation does not have 0% or 100% keyframe value
Categories
(Core :: DOM: Animation, defect, P2)
Tracking
()
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
(Keywords: perf, power, regression)
Attachments
(9 files)
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
heycam
:
review+
|
Details |
+++ This bug was initially created as a clone of Bug #1419079 +++
I am going to use bug 1419079 for optimization opacity animations, and use this for transform animations case.
Assignee | ||
Comment 1•7 years ago
|
||
From bug 1419079 comment 51;
A feasible way what I can think of was using the base style values for cumulative change hint if there is no other animation on the same element. IIRC, that is a way what Brian told me on IRC as well. But this does not work if the base style transform is 'none', and it's most common case I think, it reproduces UpdateContainingBlock, AddOrRemoveTransform and UpdateOverflow change hint. So in any way we have to handle these change hints somehow.
Given that transform animations on the compositor have been working well, even the transform animation contains 'transform:none', say something like this;
0% { transform: translateX(100px); }
50% { transform: none; }
100% { transform: translateX(-100px); }
So we can throttle those change hints as well for offscreen transform animations?
As for AddOrRemoveTransform, its purpose is to add NS_FRAME_MAY_BE_TRANSFORMED [1]. The NS_FRAME_MAY_BE_TRANSFORMED flag is also added for animation case as well [2], and in the case where the animating element doesn't yet have nsIFrame (i.e. the element is initially appended to the document or reframing happens), we post RequestRestyle(Layer) I believe. So I think AddOrRemoveTransform change hint can be throttled for offscreen animations.
As for UpdateOverflow, we probably can handle it just like UpdatePostTransformOverflow, i.e. unthrottling the animation periodically to update overflow region.
As for UpdateContainingBlock, I have no idea about the change hint yet. But again, given that it works on the compositor side, we should throttle this change hint as well for offscreen animations.
[1] https://hg.mozilla.org/mozilla-central/file/38f49346a200/layout/base/RestyleManager.cpp#l1500
[2] https://hg.mozilla.org/mozilla-central/file/38f49346a200/dom/animation/KeyframeEffectReadOnly.cpp#l1881
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
I was thinking about UpdateContainingBlock a bit more. And I start thinking it hasn't worked well on the compositor. We need test cases that are similar to ones added in bug 1187851 but having animations.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 5•6 years ago
|
||
Now I am convinced that we can throttle transform animations on visibility:hidden element even if the animation produces nsChangeHint_UpdateContainingBlock. Currently we are throttling animations on visibility:hidden element if there is no descendant element whose visibility is visible. So even if there is any absolutely or fixed positioned descendants, the descendants still is being invisible, so nsChangeHint_UpdateContainingBlock shouldn't affect at all.
Summary: Throttle transform animations on out of view elements that the transform animation does not have 0% or 100% keyframe value → Throttle transform animations on visibility:hidden element that the transform animation does not have 0% or 100% keyframe value
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•6 years ago
|
||
Now I realized that bug 1404181 made transform animations on visibility:hidden element (or inside visibility:hidden element) run on the compositor (Awesome retained display list!). So the current situation is not so bad, but unfortunately google search result case (bug 1218169) visibility:hidden element has also opacity:0 property, it seems that the retained display list didn't handle such cases. So still this bug is worth doing.
Assignee | ||
Comment 7•6 years ago
|
||
After some more learned about containing block, I am now convinced that we can throttle UpdateContainingBlock change hint in scrolled-out element too, since animation behaves as if will-change property, so when an animation is created, even if the animation in delay phase, a containing block is created for the animating CSS property (if the property generates a containing block), so we don't need to worrying about UpdateContainingBlock change hint while the animation is alive.
Summary: Throttle transform animations on visibility:hidden element that the transform animation does not have 0% or 100% keyframe value → Throttle transform animations on out of view element that the transform animation does not have 0% or 100% keyframe value
Assignee | ||
Comment 8•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
Though I am pretty sure I am on the right track, but unfortunately these patches don't optimize all animations in a google seach result in bug 1218169 comment 63. Some of animations there are optimized by these patches, but some of them aren't. I am now digging into what prevents us there. I guess it's an edge case what we can't correctly track visibility property changes in descendants, just like a todo_is[1] in test_restyles.html.
[1] https://hg.mozilla.org/mozilla-central/file/27e90ec610a4/dom/animation/test/mozilla/file_restyles.html#l882
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 8986971 [details]
Bug 1430884 - A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'.
This reftest fails to draw the scroll bar. :/
Attachment #8986971 -
Flags: review?(bbirtles)
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> Comment on attachment 8986971 [details]
> Bug 1430884 - A reftest that transform animation makes the element as a
> containing block for fixed-pos descendants even if the element is
> visibility:hidden and if the animating value is 'transform:none'.
>
> This reftest fails to draw the scroll bar. :/
Overflow area calculation or something relevant stuff for transform:none animation seems to be broken, or the value is different from transform:translateX(0px) in the first place. I will drop a keyframe at 90% there. It works locally.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > Comment on attachment 8986971 [details]
> > Bug 1430884 - A reftest that transform animation makes the element as a
> > containing block for fixed-pos descendants even if the element is
> > visibility:hidden and if the animating value is 'transform:none'.
> >
> > This reftest fails to draw the scroll bar. :/
>
> Overflow area calculation or something relevant stuff for transform:none
> animation seems to be broken, or the value is different from
> transform:translateX(0px) in the first place. I will drop a keyframe at 90%
> there. It works locally.
I had been misunderstanding that transform animations create a containing block, BUT actually it works only in the case where the animating transform value becomes non-'none' value soon after the animation started. If the transform animation is from 'none' to 'none', we don't create a containing block at all, filed bug 1470370 for that.
That's said, it doesn't affect this bug, comments should be revised though.
Assignee | ||
Comment 27•6 years ago
|
||
A new try but comments hasn't revised yet;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3732ffe37f9b6014382503b6506d81682dc82bc5
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> After some more learned about containing block, I am now convinced that we
> can throttle UpdateContainingBlock change hint in scrolled-out element too,
> since animation behaves as if will-change property, so when an animation is
> created, even if the animation in delay phase, a containing block is created
> for the animating CSS property (if the property generates a containing
> block), so we don't need to worrying about UpdateContainingBlock change hint
> while the animation is alive.
Yes, this is my understanding too. I was reading through this bug thinking exactly this so I'm glad we reached the same conclusion!
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8986966 [details]
Bug 1430884 - Use const/let in file_restyles.html.
https://reviewboard.mozilla.org/r/252204/#review259038
Attachment #8986966 -
Flags: review?(bbirtles) → review+
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8986967 [details]
Bug 1430884 - Rename unthrottling transform animations stuff.
https://reviewboard.mozilla.org/r/252206/#review259040
::: dom/animation/EffectSet.h:212
(Diff revision 1)
> - // transform animations that run on the compositor but need to be updated on
> - // the main thread periodically (e.g. so scrollbars can be updated).
> - TimeStamp mLastTransformSyncTime;
> + // This is used for the animations that are throttled restyling due to either
> + // running on the compositor or being skipped restyling because of
> + // invisibility but need to be updated on the main thread periodically (e.g.
> + // so scrollbars can be updated).
This is used for animations whose main-thread restyling is throttled either because they are running on the compositor or because they are not visible. We still need to update them on the main thread periodically, however (e.g. so scrollbars can be updated), so this tracks the last time we did that.
Attachment #8986967 -
Flags: review?(bbirtles) → review+
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8986968 [details]
Bug 1430884 - Throttle animations producing nsChangeHint_UpdateOverflow change hint if the target element is not visible.
https://reviewboard.mozilla.org/r/252208/#review259042
I'm not confident I understand all the different cases where we produce nsChangeHint_UpdateOverflow and whether each of them are ok to throttle for out-of-view elements in particular but I trust you've checked.
One spot check that might be interesting is animating the box-shadow on an out-of-view element such that the shadow reaches in into the viewport. Might we accidentally throttle that up until the first sync after it overlaps the visible viewport?
Attachment #8986968 -
Flags: review?(bbirtles) → review+
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8986969 [details]
Bug 1430884 - Factor out checking the animation on the given frame can be throttled if the frame is not visible.
https://reviewboard.mozilla.org/r/252210/#review259046
Attachment #8986969 -
Flags: review?(bbirtles) → review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8986970 [details]
Bug 1430884 - Flatten CanThrottleIfNotVisible function with early returns.
https://reviewboard.mozilla.org/r/252212/#review259048
This is so much better.
::: dom/animation/KeyframeEffect.cpp:1218
(Diff revision 1)
> + // If there is no overflow change hints, we don't need to worry unthrottling
> + // the animation periodically to update scrollbar positions for the overflow
> + // region.
> + if (!HasPropertiesThatMightAffectOverflow()) {
If there are no overflow change hints, we don't need to worry about unthrottling the animation periodically to update scrollbar positions for the overflow region.
Attachment #8986970 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #34)
> Comment on attachment 8986968 [details]
> Bug 1430884 - Throttle animations producing nsChangeHint_UpdateOverflow
> change hint if the target element is not visible.
>
> https://reviewboard.mozilla.org/r/252208/#review259042
>
> I'm not confident I understand all the different cases where we produce
> nsChangeHint_UpdateOverflow and whether each of them are ok to throttle for
> out-of-view elements in particular but I trust you've checked.
>
> One spot check that might be interesting is animating the box-shadow on an
> out-of-view element such that the shadow reaches in into the viewport. Might
> we accidentally throttle that up until the first sync after it overlaps the
> visible viewport?
Yes, it happens but only if the animation is infinite. Also such throttled animations are unthrottled every 200ms just like we do for transform animations, so it won't be a big problem.
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Though I am pretty sure I am on the right track, but unfortunately these
> patches don't optimize all animations in a google seach result in bug
> 1218169 comment 63. Some of animations there are optimized by these
> patches, but some of them aren't. I am now digging into what prevents us
> there. I guess it's an edge case what we can't correctly track visibility
> property changes in descendants, just like a todo_is[1] in
> test_restyles.html.
I found the reason why some animations in the google search results don't be throttled. That's because we do call UpdateVisibleDescendants for placeholder frames too.
A new additional patch is cmming.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e3a98f94d1e58fc012ca5251d4a790a087ab3d2
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8986971 [details]
Bug 1430884 - A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'.
https://reviewboard.mozilla.org/r/252214/#review259052
Thanks for the detailed commit message!
::: commit-message-de2de:6
(Diff revision 4)
> +Bug 1430884 - A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'. r?birtles
> +
> +As per the CSS Animations spec [1], animations must behave as if 'will-change'
> +is specified, and as per the Will Change spec [2] the element having
> +'will-change' property other than 'auto' behaves as a containing block for
> +fixed-pos descendants. This reftest tests the behavior. The reason we also
Nit: This reftest tests that behavior.
::: layout/reftests/css-animations/containing-block-on-visibility-hidden-ref.html:5
(Diff revision 4)
> +<!DOCTYPE html>
> +<style>
> +#parent {
> + visibility: hidden;
> + transform: translateX(0px);
(I guess technically, testing `will-change: transform` would be more correct here, but maybe it doesn't matter.)
::: layout/reftests/css-animations/containing-block-on-visibility-hidden.html:9
(Diff revision 4)
> +even if the animation value is 'transform:none'
> +</title>
> +<style>
> +#parent {
> + visibility: hidden;
> + animation: anim 100s step-end;
You could also just test with a delay of 100s since the will-change behavior applies during the delay phase too. In fact, that might be an even better test? What do you think?
Attachment #8986971 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 40•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #39)
> Comment on attachment 8986971 [details]
> Bug 1430884 - A reftest that transform animation makes the element as a
> containing block for fixed-pos descendants even if the element is
> visibility:hidden and if the animating value is 'transform:none'.
>
> https://reviewboard.mozilla.org/r/252214/#review259052
>
> Thanks for the detailed commit message!
>
> ::: commit-message-de2de:6
> (Diff revision 4)
> > +Bug 1430884 - A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'. r?birtles
> > +
> > +As per the CSS Animations spec [1], animations must behave as if 'will-change'
> > +is specified, and as per the Will Change spec [2] the element having
> > +'will-change' property other than 'auto' behaves as a containing block for
> > +fixed-pos descendants. This reftest tests the behavior. The reason we also
>
> Nit: This reftest tests that behavior.
>
> :::
> layout/reftests/css-animations/containing-block-on-visibility-hidden-ref.
> html:5
> (Diff revision 4)
> > +<!DOCTYPE html>
> > +<style>
> > +#parent {
> > + visibility: hidden;
> > + transform: translateX(0px);
>
> (I guess technically, testing `will-change: transform` would be more correct
> here, but maybe it doesn't matter.)
Indeed, it should be will-change:transform. I will use it.
> :::
> layout/reftests/css-animations/containing-block-on-visibility-hidden.html:9
> (Diff revision 4)
> > +even if the animation value is 'transform:none'
> > +</title>
> > +<style>
> > +#parent {
> > + visibility: hidden;
> > + animation: anim 100s step-end;
>
> You could also just test with a delay of 100s since the will-change behavior
> applies during the delay phase too. In fact, that might be an even better
> test? What do you think?
We can't do that for now because of bug 1470370, the test fails if the animations has a positive delay.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 50•6 years ago
|
||
mozreview-review |
Comment on attachment 8986972 [details]
Bug 1430884 - Throttle nsChangeHint_UpdateContainingBlock on invisible element.
https://reviewboard.mozilla.org/r/252216/#review259054
::: commit-message-907b0:3
(Diff revision 4)
> +When an animation for a CSS property which might cause a containing block for
> +its descendants, the animation creates a containing block even if the property
> +value doesn't create a containing block, e.g. transform:none. So it doesn't
> +matter that UpdateContainingBlock change hint doesn't happen during animation
> +restyles, that means that we can throttle the change hint if the animating
> +element isn't visible.
Does this explanation make sense?
When an animation targets a CSS property that could cause a containing block to be generated for its descendants, this containing block must be generated even if the particular property values used by the animation would not normally trigger generation of a containing block (e.g. 'transform: none'). This is due to the implicit application of will-change defined in CSS Animations[1] and Web Animations[2].
Since this containing block is generated at the start of the animation, we can throttle animations that produce the UpdateContainingBlock change hint for animations that are not visible since they shouldn't have any further side effects beyond the generation of containing blocks (which have already happened).
[1] https://drafts.csswg.org/css-animations/#animations
[2] https://drafts.csswg.org/web-animations-1/#side-effects-section
::: commit-message-907b0:10
(Diff revision 4)
> +value doesn't create a containing block, e.g. transform:none. So it doesn't
> +matter that UpdateContainingBlock change hint doesn't happen during animation
> +restyles, that means that we can throttle the change hint if the animating
> +element isn't visible.
> +
> +Unfortuntely perspective animations start with 'none' and transform animations
Nit: s/Unfourtuntely/Unfortunately/
s/start with/starting with/
::: commit-message-907b0:12
(Diff revision 4)
> +1470370), but it doesn't mean that the bug blocks us from doing this
> +optimization because the bug happens regardless of the element visibility.
s/, but it doesn't.../. That doesn't block the optimization in this patch, however, since those bugs occur regardless of element visibility./
?
Attachment #8986972 -
Flags: review?(bbirtles) → review+
Comment 51•6 years ago
|
||
mozreview-review |
Comment on attachment 8986973 [details]
Bug 1430884 - Throttle transform animations without %0 or 100% keyframe.
https://reviewboard.mozilla.org/r/252218/#review259050
::: dom/animation/KeyframeEffect.cpp:1624
(Diff revision 4)
> + // In the case the property is transform, we want to forcibly optimize
> + // it even if we can't precompute the cumulative change hints since it's
> + // the second common property for animations, so we suppose the worst
> + // case here, i.e. all possible change hints happen.
We try a little harder to optimize transform animations simply because they are so common (the second-most commonly animated property at the time of writing). So, if we encounter a transform segment that needs composing with the underlying value, we just add all the change hints a transform animation is known to be able to generate.
::: dom/animation/KeyframeEffect.cpp:1626
(Diff revision 4)
> - mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible;
> + mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible;
> - return;
> + return;
> - }
> + }
> + // In the case the property is transform, we want to forcibly optimize
> + // it even if we can't precompute the cumulative change hints since it's
> + // the second common property for animations, so we suppose the worst
s/second/second-most/
::: layout/style/nsStyleStruct.cpp:3822
(Diff revision 4)
> if (HasTransformStyle() != aNewData.HasTransformStyle()) {
> // We do not need to apply nsChangeHint_UpdateTransformLayer since
> // nsChangeHint_RepaintFrame will forcibly invalidate the frame area and
> // ensure layers are rebuilt (or removed).
> + //
> + // XXX: If we add a new change hint for transform changes here or in
I'd probably prefer s/XXX/NOTE/ since we mostly seem to use XXX in Gecko to mean TODO. It's up to you though.
::: layout/style/nsStyleStruct.cpp:3822
(Diff revision 4)
> + // XXX: If we add a new change hint for transform changes here or in
> + // CompareTransformValues(), we have to modify
> + // KeyframeEffect::CalculateCumulativeChangeHint too!
I suppose we should add a similar comment to CompareTransformValues() too.
Attachment #8986973 -
Flags: review?(bbirtles) → review+
Comment 52•6 years ago
|
||
mozreview-review |
Comment on attachment 8987389 [details]
Bug 1430884 - Don't call UpdateVisibleDescendants for placeholder frames.
https://reviewboard.mozilla.org/r/252638/#review259056
This seems fine to me but I'm less familiar with frame construction so I'd like either heycam or emilio to check this.
::: layout/generic/nsFrame.cpp:725
(Diff revision 1)
> + if (!IsPlaceholderFrame()) {
> - UpdateVisibleDescendantsState();
> + UpdateVisibleDescendantsState();
> -}
> + }
This should have a comment describing why placeholder frames are special. (Perhaps just move your comment from the commit message here?)
Attachment #8987389 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 53•6 years ago
|
||
Comment on attachment 8987389 [details]
Bug 1430884 - Don't call UpdateVisibleDescendants for placeholder frames.
Cameron?
Attachment #8987389 -
Flags: review?(cam)
Comment 54•6 years ago
|
||
mozreview-review |
Comment on attachment 8987389 [details]
Bug 1430884 - Don't call UpdateVisibleDescendants for placeholder frames.
https://reviewboard.mozilla.org/r/252638/#review259082
Thanks, yes looks good to me, for the same reason we walk through placeholder frames (and don't inspect the placeholders themselves) in HasNoVisibleDescendants, and use GetInFlowParent() in nsIFrame::UpdateVisibleDescendantsState, skipping placeholders. Do add a comment though.
Attachment #8987389 -
Flags: review?(cam) → review+
Assignee | ||
Comment 55•6 years ago
|
||
mozreview-review |
Comment on attachment 8986968 [details]
Bug 1430884 - Throttle animations producing nsChangeHint_UpdateOverflow change hint if the target element is not visible.
https://reviewboard.mozilla.org/r/252208/#review259086
::: dom/animation/test/mozilla/file_restyles.html:1744
(Diff revision 2)
> + await waitForAnimationReadyToRestyle(animation);
> +
> + const markers = await observeStyling(5);
> +
> + is(markers.length, 0,
> + 'Outline offset animation running on the main-thread on invisible ' +
> + 'element should be throttled');
This test fails intermittently on Android, I am going to disable this on Android and will take care of later in a follow up.
Assignee | ||
Comment 56•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> Comment on attachment 8986968 [details]
> Bug 1430884 - Throttle animations producing nsChangeHint_UpdateOverflow
> change hint if the target element is not visible.
>
> https://reviewboard.mozilla.org/r/252208/#review259086
>
> ::: dom/animation/test/mozilla/file_restyles.html:1744
> (Diff revision 2)
> > + await waitForAnimationReadyToRestyle(animation);
> > +
> > + const markers = await observeStyling(5);
> > +
> > + is(markers.length, 0,
> > + 'Outline offset animation running on the main-thread on invisible ' +
> > + 'element should be throttled');
>
> This test fails intermittently on Android, I am going to disable this on
> Android and will take care of later in a follow up.
Filed bug 1470798.
Assignee | ||
Comment 57•6 years ago
|
||
This will be a final try (I did push for MacOSX only though);
https://treeherder.mozilla.org/#/jobs?repo=try&revision=658e2524d2cadb192d17a7eef7819645a433594a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 65•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8986972 [details]
Bug 1430884 - Throttle nsChangeHint_UpdateContainingBlock on invisible element.
https://reviewboard.mozilla.org/r/252216/#review259054
> Does this explanation make sense?
>
>
>
> When an animation targets a CSS property that could cause a containing block to be generated for its descendants, this containing block must be generated even if the particular property values used by the animation would not normally trigger generation of a containing block (e.g. 'transform: none'). This is due to the implicit application of will-change defined in CSS Animations[1] and Web Animations[2].
>
> Since this containing block is generated at the start of the animation, we can throttle animations that produce the UpdateContainingBlock change hint for animations that are not visible since they shouldn't have any further side effects beyond the generation of containing blocks (which have already happened).
>
> [1] https://drafts.csswg.org/css-animations/#animations
> [2] https://drafts.csswg.org/web-animations-1/#side-effects-section
Thanks! I hadn't noticed the Web Animations spec. :)
Comment 66•6 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cec68a5b01da
Use const/let in file_restyles.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/52be07f10eaa
Rename unthrottling transform animations stuff. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1e047fbcae2a
Throttle animations producing nsChangeHint_UpdateOverflow change hint if the target element is not visible. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6dbae2b8957d
Factor out checking the animation on the given frame can be throttled if the frame is not visible. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e7e8977e0e92
Flatten CanThrottleIfNotVisible function with early returns. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4505e3f87b2e
A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'. r=birtles
https://hg.mozilla.org/integration/autoland/rev/39e1c4e3c8c9
Throttle nsChangeHint_UpdateContainingBlock on invisible element. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c3f9f3f66a98
Throttle transform animations without %0 or 100% keyframe. r=birtles
https://hg.mozilla.org/integration/autoland/rev/262dbc8daac1
Don't call UpdateVisibleDescendants for placeholder frames. r=birtles,heycam
Comment 67•6 years ago
|
||
Backed out 9 changesets (bug 1430884) for frequently failing dom/animation/test/mozilla/test_restyles.html on android-em-4-3-armv7-api16 debug
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=262dbc8daac150e7c62782356093c6d171db7e0b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=pending&filter-resultStatus=running
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184649336&repo=autoland&lineNumber=1415
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3de09bc10e6ddc05348debebd754d296d648363a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=pending&filter-resultStatus=running
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 68•6 years ago
|
||
Gah, on Android all test cases added here are intermittent. I am going to disable them on Android for now.
Flags: needinfo?(hikezoe)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 72•6 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29648dc20a59
Use const/let in file_restyles.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ab4d2d03fc04
Rename unthrottling transform animations stuff. r=birtles
https://hg.mozilla.org/integration/autoland/rev/acde077a83d8
Throttle animations producing nsChangeHint_UpdateOverflow change hint if the target element is not visible. r=birtles
https://hg.mozilla.org/integration/autoland/rev/814d65e3d170
Factor out checking the animation on the given frame can be throttled if the frame is not visible. r=birtles
https://hg.mozilla.org/integration/autoland/rev/21a95abbe05a
Flatten CanThrottleIfNotVisible function with early returns. r=birtles
https://hg.mozilla.org/integration/autoland/rev/756afbbd2235
A reftest that transform animation makes the element as a containing block for fixed-pos descendants even if the element is visibility:hidden and if the animating value is 'transform:none'. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1abd6e3e1cd4
Throttle nsChangeHint_UpdateContainingBlock on invisible element. r=birtles
https://hg.mozilla.org/integration/autoland/rev/0afedcfb3241
Throttle transform animations without %0 or 100% keyframe. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c0b21495ab92
Don't call UpdateVisibleDescendants for placeholder frames. r=birtles,heycam
Comment 73•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29648dc20a59
https://hg.mozilla.org/mozilla-central/rev/ab4d2d03fc04
https://hg.mozilla.org/mozilla-central/rev/acde077a83d8
https://hg.mozilla.org/mozilla-central/rev/814d65e3d170
https://hg.mozilla.org/mozilla-central/rev/21a95abbe05a
https://hg.mozilla.org/mozilla-central/rev/756afbbd2235
https://hg.mozilla.org/mozilla-central/rev/1abd6e3e1cd4
https://hg.mozilla.org/mozilla-central/rev/0afedcfb3241
https://hg.mozilla.org/mozilla-central/rev/c0b21495ab92
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 74•6 years ago
|
||
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6caca46bd71
followup: Fix some typos in file_restyles.html. r=me
Assignee | ||
Comment 75•6 years ago
|
||
Thanks Emilio. :)
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Comment 77•6 years ago
|
||
bugherder |
Comment 78•6 years ago
|
||
Tagging this as a QF P1 bug (targeted at 64, though it's already done, yay!), since this seems like it'll help with performance on some high-usage properties like Google, per comment 6 / comment 38.
Whiteboard: [qf:p1:f64]
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64]
You need to log in
before you can comment on or make changes to this bug.
Description
•