1237454 - [Power] Optimize animations in visibility:hidden elements

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

+++ This bug was initially created as a clone of Bug #1166500 +++

We are not going to handle animations on/inside visibility:hidden elements.
This bug aims to optimize those animations.  I believe we can optimize at least animations on child elements which are inside visibility:hidden elements.

Comment 1

[Daniel Holbert [:dholbert]]

This seems doable, for animations on CSS properties that don't influence layout.

I don't think we could optimize away animations on CSS properties that *do* influence layout, though (e.g. "width").

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daniel Holbert [:dholbert] (largely AFK until June 28) from comment #1)
> This seems doable, for animations on CSS properties that don't influence
> layout.
> 
> I don't think we could optimize away animations on CSS properties that *do*
> influence layout, though (e.g. "width").

We have already KeyframeEffectReadOnly::CanIgnoreIfNotVisible() to check whether animation influences layout or not.

A missing part of this bug is a function to check whether all descendant elements are visibility:hidden or not.

A possible solution I can think of is:

1. Add a flag in nsStyleContext to represent that all descendants are visibility:hidden.
2. Set the flag true initially.
3. In nsStyleContext::ApplyStyleFixups(), check StyleVisibility()->IsVisible(), if it's true, then the flags of all ancestors are set to false.
4. Add a function to return the flag in nsStyleContext.
5. Throttle animations if the flag is true and CanIgnoreIfNotVisible() is true.

Cameron, does this solution sound feasible?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Throttle animations in elements that all descendants of the element have visivility:hidden

For reference I am attaching a patch that I wrote before.

I am not sure this patch can be applied on current trunk.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Similar issue for Blink. https://bugs.chromium.org/p/chromium/issues/detail?id=659570

Comment 5

[Karl Dubost💡 :karlcow]

Should I open a different issue for this one.
https://webcompat.com/issues/12827

the dots are out of sight. Animation is constantly running making 3 dots varying from scale(0) to scale(1) asynchronously. CPU goes from 2% to 50% maintained.

Comment 6

[Brian Birtles (:birtles)]

That sounds like a different bug since it's not about visibility:hidden (and bug 1242969 might be related).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thanks for checking.  I don't have an account for quora.com so I can't confirm the animation by myself for now.
If the reason for the out of sight is that the animating element is scrolled out from the view, it will be fixed by bug 1190721.  We haven't yet optimized transform animations in scrolled out elements.
If the reason is that the animating element is visibility:hidden ancestor, this bug is for it.

Oops, collision.  Yep, if the animation is out of sight for other reason, we might need other optimization there like bug 1242969.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8777151 [details] [diff] [review]
Throttle animations in elements that all descendants of the element have visivility:hidden

I've been thinking about this for stylo.  In stylo, as far as I know, while styling an element, we can't mutate any ancestors for the element (i.e. fixing up the ancestors which are visibility:hidden) when we notice the element is not visibility:hidden in the visibility:hidden subtree.  A way I can think of using a sequential task.  I am not sure we have other efficient ways.

Comment 9

[Elhem Enohpi]

Attachment: HTML test page for hidden "loading spinners".

I was just testing this out on the Quantum beta on my Macbook, here are some rough results using the attached test page.

The test page shows some typical examples of the "loading spinners" used on virtually every modern website, which are usually kept in a "visibility: hidden" element when not being displayed. There are often several of them on one page, which can quickly add up to 100% CPU usage for tiny hidden animations, at least on a Mac.

I didn't see any significant difference in CPU usage between Firefox 52 ESR, FF 56.0.2, and FF 57.0b14 with or without Servo enabled. The following numbers are roughly as reported by the CPU meter of iStat Menus; 100% is one core. I also included the values for the windowserver process:

CPU usage, on my 2012 MacBook Air with macOS 10.12, when animation is:

displaying: 19% + windowserver 16%
hidden: 32% + windowserver 4%
scrolled away: 21% + windowserver 14%
hidden & scrolled away: 32% + windowserver 4%
none: < 1% + windowserver 2%

As you can see, Firefox's CPU is actually about 50% higher when they're hidden! Also the usage when scrolled offscreen is considerably higher than I would expect from Bug #1166500, and furthermore that fix seems to have no effect if the animation is visibility: hidden.

As a comparison, the results for Safari are:

displaying: 7% + windowserver 16%
hidden: 8% + windowserver 8%
scrolled away : 6% + windowserver 8%
hidden & scrolled: same as scrolled
none: < 1% + windowserver 2%

Interestingly, Safari's CPU usage is about the same when the animation is displaying, hidden, or scrolled offscreen; with only an 8% increase in the windowserver process when actually displaying. The main difference, aside from not having FF's large CPU increase when hidden, is that it's just overall more efficient, about 1/3 to 1/2 of Firefox's usage.

At first I tried to track down and block hidden animations with uBlock, but it was a losing battle. Because of the widespread use of these hidden "spinner" animations, and the excessive CPU used on the Mac, to save battery power I've disabled (restricted to one loop iteration) all infinite CSS animations with this greasemonkey script:

// ==UserScript==
// @name        Stop CSS Infinite Animations
// @namespace   Elhem.Enohpi
// @include     *
// @version     1.0
// @grant       GM_addStyle
// ==/UserScript==

// Make all CSS animations run only once:
GM_addStyle("*, *::before, *::after {animation-iteration-count: 1 !important}");

Comment 10

[Elhem Enohpi]

Sorry, the attachment above doesn't seem to work by clicking on it, try "save link as" and opening as a file...

Comment 11

[Elhem Enohpi]

I just got FF58 on the developer channel. There seems to be a significant improvement, I'm no longer seeing the large increase in CPU when the animation is hidden. It's now the same or slightly less than when it's being displayed. It's still about double what Safari and Vivaldi use when hidden:

FF58.0b1
displaying: 21% + windowserver 16%
hidden: 19% + windowserver 4%
scrolled away: 19% + windowserver 7%
hidden & scrolled: same as hidden
none: < 1% + windowserver 2%

Vivaldi 1.12.955.48:
displaying: 27% + windowserver 16%
hidden: 9% + windowserver 2%
scrolled away: same as hidden
hidden & scrolled: same as hidden
none: < 1% + windowserver 2%

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I came up with another idea instead of using sequential task.  The idea is just adding a new change hint for visibility property change.  I think it's much simpler.  Unfortunately there is no room for the new change hint in current nsChangeHint (uint32_t), so we need to change nsChangeHint to uint64_t.  Cameron told me on IRC that 'it would be OK if you need to'.  (I think it will be changed sooner or later)

Also note that FWIW, chrome does check whether all descendants are invisible or not [1]

[1] https://chromium.googlesource.com/chromium/src.git/+/34720eee6bf1fb2da346421b26cadb16052d113a/third_party/WebKit/Source/core/paint/PaintLayer.cpp#699

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I was thinking that to optimize this properly we need to store visible descendants count on each element and decrease/increase it when a descendant gets visible/invisible respectively.  I meant I was thinking we can't avoid overkills without counting.  But after some thoughts and writing test cases, I think we don't overkill without counting.  So I'd take the approach which doesn't count visible descendants here in this bug , and later in other bug fix completely.

FWIW, here is a test case that can't be optimized by the approach what I am trying to do here;

  div.style.visibility = 'hidden'; // the 'div' has no children at this moment
  div.animate(..);
  // The animation is throttled

  // in later frame
  div.appendChild(visibleChild);
  // The animation isn't throttled

  // in later frame
  visibleChild.style.visibility = 'hidden';
  // Now the animation should throttle again, but actually it's not.

To optimize this without counting visible descendants, we need to check all siblings of the |visibleChild| have no visible children.  If the visiblity:hidden element is the grandparent, we need to walk up ancestors and check all siblings there.  It's horrible. :/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf16465620a8b9e1036901227bfe29a81381d3d

I could confirm that the Google issue (bug 1218169) will be solved partially without counting visible descendants,  'partially' I meant is that some opacity animations on the Google search results don't specify 100% keyframe value so that we can't throttle the opacity animations due to bug 1419079.  So I'd like to land the fix without counting visible descendants here in this bug, and will fix other issues one by one.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Add VisibilityChange change hint.

This new change hint doesn't influence layout so that it can be regarded
as nsChangeHint_Hints_CanIgnoreIfNotVisible.  Note that if visibility changed
from collapse or to collapse, we set NS_STYLE_HINT_REFLOW separetely.

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

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

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

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

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

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Throttle animations on visibility:hidden element.

This patch does basically throttle animations on visibility:hidden element
and unthrottle it once the animating element became visible or a child of the
animating element became visible.  But still there are some cases that we don't
throttle such animations perfectly.  For example;

  div.style.visibility = 'hidden'; // the 'div' has no children at this moment
  div.animate(..);
  // The animation is throttled

  div.appendChild(visibleChild);
  // The animation isn't throttled

  visibleChild.style.visibility = 'hidden';
  // Now the animation should throttle again, but actually it's not.

To throttle this case properly, when the |visibleChild|'s visibility changed
to hidden, we need to do either

 1) Check all siblings of the |visibleChild| have no visible children

or

 2) The parent element stores visible children count somewhere and decrease it
    and check whether the count is zero

To achieve 1) we need to walk up ancestors and their siblings, actually it's
inefficient.

2) is somewhat similar to what we already do for animating images but it's hard
to reuse it for CSS animations since it does not take into account that
descendants' visibilities.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Traverse all ancestor siblings when visibility property changed to hidden

FWIW, here is a patch to throttle visibility:hidden animations completely (without counting visible descendants).

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218724

I think I understand this but I'll wait for you to check it over once more and explain a little more about the operation.

Until then I just have some minor grammar/naming feedback.

::: commit-message-92e0f:3
(Diff revision 1)
> +This patch does basically throttle animations on visibility:hidden element
> +and unthrottle it once the animating element became visible or a child of the
> +animating element became visible.  But still there are some cases that we don't
> +throttle such animations perfectly.  For example;

Just to check I've understood the meaning of this comment, could we write it as:

"This patch throttles most animations on visibility:hidden elements where all descendant elements are also hidden. It unthrottles them once the animated element, or one of its descendants, becomes visible.

There are some cases that it doesn't throttle, however. For example:"

?

::: commit-message-92e0f:16
(Diff revision 1)
> +
> +  div.appendChild(visibleChild);
> +  // The animation isn't throttled
> +
> +  visibleChild.style.visibility = 'hidden';
> +  // Now the animation should throttle again, but actually it's not.

(Nit: s/should throttle/should be throttled/)

::: commit-message-92e0f:19
(Diff revision 1)
> +
> +  visibleChild.style.visibility = 'hidden';
> +  // Now the animation should throttle again, but actually it's not.
> +
> +To throttle this case properly, when the |visibleChild|'s visibility changed
> +to hidden, we need to do either

Nit: s/we need to do either/we would need to either/

(Just to make it clear that this is describing something we *don't* do in this patch.)

::: layout/base/RestyleManager.cpp:1724
(Diff revision 1)
> +
> +      if (hint & nsChangeHint_VisibilityChange) {
> +        frame->UpdateVisibleDescendantsState();
> +      }

(Style nit: The other change hint checks here don't include a blank line between them. I think we can drop the blank line here.)

::: layout/generic/nsFrame.cpp:735
(Diff revision 1)
>    DidSetStyleContext(nullptr);
>  
>    if (::IsXULBoxWrapped(this))
>      ::InitBoxMetrics(this, false);
> +
> +  // For newly created frame, we need to update this frame's visibility state.

(Nit: For a newly created...)

::: layout/generic/nsFrame.cpp:736
(Diff revision 1)
>  
>    if (::IsXULBoxWrapped(this))
>      ::InitBoxMetrics(this, false);
> +
> +  // For newly created frame, we need to update this frame's visibility state.
> +  // Usually we update the state when the frame is restyled and has

(Nit: ... is restyled and has a VisibilityChange...)

::: layout/generic/nsFrame.cpp:11331
(Diff revision 1)
> +static bool
> +HasNoVisibleDescendants(const nsIFrame* aFrame)
> +{
> +  for (nsIFrame::ChildListIterator lists(aFrame);
> +       !lists.IsDone();
> +       lists.Next()) {
> +    for (nsIFrame* f : lists.CurrentList()) {
> +      if (f->IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;
> +      }
> +    }
> +  }
> +  return true;
> +}
> +
> +void
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.
> +    nsIFrame* ancestor;
> +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetFlattenedTreeParentPrimaryFrame()) {
> +      ancestor->mHasNoVisibleDescendants = false;
> +    }
> +  } else {
> +    mHasNoVisibleDescendants = HasNoVisibleDescendants(this);
> +  }
> +}

I had trouble following this logic at first. I think the two problems were:

1. the naming mHasNoVisibleDescendants (see comment below)
2. I didn't understand that all descendants will get the relevant change hint and run this in turn.

I'm a bit concerned that if we make a large subtree visibility:hidden, that the result of processing all the change hints on descendants will mean we end up traversing the entire subtree. Is that the case?

I think you said you were going to check this code over once more. While you're at it, could you explain the steps that take place when we set visibility:hidden on frame B in the graph below:

      A
     / \
    B   C
   / \
  D   E
 /
F

::: layout/generic/nsFrame.cpp:11350
(Diff revision 1)
> +
> +void
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.

(Nit: Notify invisible...)

::: layout/generic/nsIFrame.h:4360
(Diff revision 1)
> +  /**
> +   * True if this frame has no visible descendants.
> +   */
> +  bool mHasNoVisibleDescendants : 1;

As discussed on IRC, the negative in this name makes this code a little hard to follow.

One suggestion is mAllDescendantsAreInvisible ?

And a comment such as:

/**
 * True if we are certain that all descendants are not visible.
 *
 * This flag is conservative in that it might sometimes be false
 * even if, in fact, all descendants are invisible.
 */

Comment 23

[Boris Chiou [:boris]]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

https://reviewboard.mozilla.org/r/213034/#review218744

::: dom/animation/test/mozilla/file_restyles.html:641
(Diff revision 1)
>         'view by resizing');
>  
>      await ensureElementRemoval(parentElement);
>    });
>  
> -  add_task(async function no_restyling_main_thread_animations_in_visiblily_hidden_element() {
> +  add_task(async function restyling_animations_on_visiblily_hidden_element() {

nit:
I guess we should mention the "change" of visibility: e.g.
`restyling_animations_on_visiblily_hidden_element_which_gets_visible()`.

This might be better, I think.

::: dom/animation/test/mozilla/file_restyles.html:658
(Diff revision 1)
> +       'Animations running that was on visibility hidden element should ' +
> +       'not throttle restyling any more');

nit:

Also here. It's better to mention: this is a "visibility hidden element which gets visible"

Otherwise, the description might be misread.

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

https://reviewboard.mozilla.org/r/213036/#review218746

::: dom/animation/test/mozilla/file_restyles.html:664
(Diff revision 1)
>         'not throttle restyling any more');
>  
>      await ensureElementRemoval(div);
>    });
>  
> +  add_task(async function restyling_animations_in_visiblily_hidden_parent() {

nit:

`restyling_animations_in_visiblily_changed_parent` or others which mention we change the visibility might be better.

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

https://reviewboard.mozilla.org/r/213038/#review218754

::: dom/animation/test/mozilla/file_restyles.html:698
(Diff revision 1)
>  
>      await ensureElementRemoval(parentDiv);
>    });
>  
> +  add_task(
> +    async function restyling_animations_on_visiblily_hidden_element() {

This test looks good to me, but we could change the name to something like:
`restyling_animations_on_visibility_hidden_element with visibility_changed_children` or others, so we could understand we are trying to change the visibility of the children.

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

https://reviewboard.mozilla.org/r/213040/#review218764

Comment 27

[Boris Chiou [:boris]]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218766

Not sure if you will add more tests according to Brian's comment, so r=me only on the current test change. Besides, I believe Brian and Cameron will review the rest part. :)

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218812

::: layout/generic/nsFrame.cpp:11352
(Diff revision 1)
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.
> +    nsIFrame* ancestor;
> +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();

FWIW, I'd use `GetInFlowParent` instead. `GetFlattenedTreeParentPrimaryFrame` is bogus for `display: contents`.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Clearing review request for now since the change hint is not handled what I expected, I need to investigate it.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> Clearing review request for now since the change hint is not handled what I
> expected, I need to investigate it.

Oh well, it's working as expected.  My debug print was wrong. :/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #22)
> (Diff revision 1)
> > +static bool
> > +HasNoVisibleDescendants(const nsIFrame* aFrame)
> > +{
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> > +      if (f->IsVisibleOrMayHaveVisibleDescendants()) {
> > +        return false;
> > +      }
> > +    }
> > +  }
> > +  return true;
> > +}
> > +
> > +void
> > +nsIFrame::UpdateVisibleDescendantsState()
> > +{
> > +  if (StyleVisibility()->IsVisible()) {
> > +    // Notify to invisible ancestors that a visible descendant exists now.
> > +    nsIFrame* ancestor;
> > +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();
> > +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> > +         ancestor = ancestor->GetFlattenedTreeParentPrimaryFrame()) {
> > +      ancestor->mHasNoVisibleDescendants = false;
> > +    }
> > +  } else {
> > +    mHasNoVisibleDescendants = HasNoVisibleDescendants(this);
> > +  }
> > +}
> 
> I had trouble following this logic at first. I think the two problems were:
> 
> 1. the naming mHasNoVisibleDescendants (see comment below)
> 2. I didn't understand that all descendants will get the relevant change
> hint and run this in turn.
> 
> I'm a bit concerned that if we make a large subtree visibility:hidden, that
> the result of processing all the change hints on descendants will mean we
> end up traversing the entire subtree. Is that the case?
> 
> I think you said you were going to check this code over once more. While
> you're at it, could you explain the steps that take place when we set
> visibility:hidden on frame B in the graph below:
> 
>       A
>      / \
>     B   C
>    / \
>   D   E
>  /
> F

Yeah, I did overlook that case, e.g. an element is initially visible and has a child whose visibility is 'inherit' and later, changing the parent element visibility to hidden.  This case is not optimized with the current patch.  In a previous version of the patch, there were code that traverses down all descendants, but after some local running test cases, I noticed all test cases I had passed without the traversing down all descendants, so I did drop the code.  Though actually there were a lack of test case that is not optimized. 

I added a lack test case in the last commit and push a try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1219c1b807c5f58feac10c0099a099b138fefac5

Note that if the element visibility is initially 'hidden', then all descendants' mAllDescendantsAreInvisible is set to true, since it's set in nsIFrame::Init(). So it's optimized.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Using GetInFlowParent caused a crash in a devtool test.  It seems that it should be called from UnbindFromTree().  I did add a UpdateVisibleDescendantsState [1] in UnbindFromTree() to make a test case (throttling again when a visible child is removed) [2] pass.   But the case is not a big deal, so I will drop the call in this bug.

A new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4114c32f5807d681bf72f02f67ecafb531268978

[1] https://hg.mozilla.org/try/rev/849b77932773c67a4c29ae8dbce0f35e471be945#l3.13
[2] https://hg.mozilla.org/try/rev/849b77932773c67a4c29ae8dbce0f35e471be945#l2.154

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213032/diff/1-2/

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213034/diff/1-2/

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213036/diff/1-2/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213038/diff/1-2/

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213040/diff/1-2/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213042/diff/1-2/

Comment 39

[Brian Birtles (:birtles)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

https://reviewboard.mozilla.org/r/213036/#review219150

::: dom/animation/test/mozilla/file_restyles.html:666
(Diff revision 2)
>  
>        await ensureElementRemoval(div);
>      }
>    );
>  
> +  add_task(async function restyling_animations_in_visiblily_changed_parent() {

s/visiblily/visibility/

Comment 40

[Brian Birtles (:birtles)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

https://reviewboard.mozilla.org/r/213034/#review219152

::: dom/animation/test/mozilla/file_restyles.html:642
(Diff revision 2)
>  
>      await ensureElementRemoval(parentElement);
>    });
>  
> -  add_task(async function no_restyling_main_thread_animations_in_visiblily_hidden_element() {
> +  add_task(
> +    async function restyling_animations_on_visiblily_hidden_element_which_gets_visible() {

s/visiblily/visibility/

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review219172

::: layout/generic/nsFrame.cpp:11334
(Diff revision 2)
> +  for (nsIFrame::ChildListIterator lists(aFrame);
> +       !lists.IsDone();
> +       lists.Next()) {
> +    for (nsIFrame* f : lists.CurrentList()) {

Say we have:

<div style="position: absolute; visibility: hidden;">
  <div style="float: left; visibility: visible">
    hello
  </div>
</div>

Will we find the child <div>'s frame in these loops, or only its placeholder frame?  Do we have to follow the placeholder to the OOF frame to correctly return false?

Similarly, is it unneccessary to look at the OOF child lists of aFrame?  (Not a correctness issue, but we would unnecessarily not perform the optimization.)

Comment 42

[Brian Birtles (:birtles)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review219148

As discussed on IRC, I think it's probably worth investigating other approaches to handle large visibility:hidden subtrees (or, perhaps more accurately, animations in visibility:hidden subtrees that have grandchildren). But if this fixes the Google issues, and doesn't (so far as I can tell) have pathological performance cases, we should probably land this now.

::: commit-message-92e0f:45
(Diff revision 2)
> +  div.appendChild(child);
> +
> +  div.animate(..); // the 'div' is visible
> +  // The animation isn't throttled since the animating element is visible
> +
> +  div.style.visiblily = 'hidden';

s/visiblily/visibility/

(Probably worth doing an `rg visiblily` on your tree just in case there are other instances!)

::: layout/generic/nsFrame.cpp:11330
(Diff revision 2)
>    }
>  
>    return result;
>  }
>  
> +// Returns true if we can guarantee there is no visible descendants.

(Nit: s/there is/there are/)

::: layout/generic/nsIFrame.h:4098
(Diff revision 2)
>  
> +  // Returns true if this frame is visible or may have visible descendants.
> +  bool IsVisibleOrMayHaveVisibleDescendants() const {
> +    return !mAllDescendantsAreInvisible || StyleVisibility()->IsVisible();
> +  }
> +  // Update mAllDescendantsAreInvisible flag for this frame and ancestors.

"Update mAllDescendantsAreInvisible flag for this frame and ancestors (if this frame is visible)."

?

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Gosh, I did not notice this Cameron's comment..

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review219172
> 
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?
> Gosh, I did not notice this Cameron's comment..

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review219172
> 
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?
> 
> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

Oh right. Indeed, I missed the out-of-flow cases (honestly I usually miss the case since I am not yet familiar with frames), I will check the case and will revise my patches and send review request again. :)

Thanks for the review!
> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

Oh right. Indeed, I missed the out-of-flow cases (honestly I usually miss the case since I am not yet familiar with frames), I will check the case and will revise my patches and send review request again. :)

Thanks for the review!

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Something weird thing happened..

Comment 45

[Hiroyuki Ikezoe (:hiro)]

I could get back this bug finally. :)

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?

I did check the test case. You are absolutely right.  We have to follow the placeholder there.

> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

IIUC, it's not necessary if we correctly follow the placeholder then find the real frame.

Anyway, I will add a test case that needs to track the placeholder.

Thank you so much!

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213032/diff/2-3/

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213034/diff/2-3/

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213036/diff/2-3/

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213038/diff/2-3/

Comment 50

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213040/diff/2-3/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213042/diff/2-3/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213032/diff/3-4/

Comment 53

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213034/diff/3-4/

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213036/diff/3-4/

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213038/diff/3-4/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213040/diff/3-4/

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213042/diff/3-4/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

I did forget to mend visibilily typos there. :)

Emilio, could you please take a time to look the first and the last patches here?  I don't want to interfere heycam's rest days for now. :)  Thanks!

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Note that my intention for this bug is to fix google search result page case (bug 1218169).  I know the patche here is not perfect but I think it fixes some amount of cases (most cases?) in the wild.

Comment 60

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

https://reviewboard.mozilla.org/r/213032/#review223534

::: layout/base/nsChangeHint.h:254
(Diff revision 4)
> +   * Indicates that the visiblity property changed.
> +   * This change hint is used for skip restyling for animations on
> +   * visibility:hidden elements in the case where the elements have no visible
> +   * descendants.
> +   */
> +  nsChangeHint_VisibilityChange = 1u << 31,

Phew, the very last one...

Comment 61

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223536

::: layout/generic/nsFrame.cpp:742
(Diff revision 4)
> +
> +  // For a newly created frame, we need to update this frame's visibility state.
> +  // Usually we update the state when the frame is restyled and has a
> +  // VisibilityChange change hint but we don't generate any change hints for
> +  // newly created frames.
> +  UpdateVisibleDescendantsState();

If we manage to do it, there'd be no dependencies on the style tree of our descendants being up-to-date, we could even move this to stop being a hint (and move it to `nsStyleContext::DidSetStyleContext` or something similar...

::: layout/generic/nsFrame.cpp:11359
(Diff revision 4)
> +            IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;

I'm somewhat concerned about this loop... Since we post changes bottom-up, if I change a visibility: visible element to visibility: hidden, we're doing tons of subtree walks suddenlty, right?

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

Why can't we rely on our children getting to notify us whenever they're visible, and assume `true` otherwise? That is, don't touch it when visibility changes?

Is it just to catch dynamic changes to visibility on children? If so, that looks kinda like a hack...

::: layout/generic/nsIFrame.h:4363
(Diff revision 4)
>  
> +  /**
> +   * True if we are certain that all descendants are not visible.
> +   *
> +   * This flag is conservative in that it might sometimes be false even if, in
> +   * fact, all descendants are invisible.

Putting an example of this would be nice.

Comment 62

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223540

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

I see, so when _our_ visibility changes to `hidden`, there's nothing that could possibly change the flag back to true... Hmm

Comment 63

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223546

My last question is how does this play with frame continuations and such. I guess animations only look at primary frames? If so, this is fine I guess, but we never call `UpdateVisibilityState` on the later continuations... Should we?

::: layout/generic/nsFrame.cpp:11359
(Diff revision 4)
> +            IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;

Also, I cannot read, and this only traverses direct children, so it's not so concerning I guess... Though it would if we throttled all the cases we should throttle I think...

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

Ok, so after thinking a bit more thoroughly, there's no great thing I can think of that doesn't involve depending on the style tree on the children to be up-to-date...

I think it's very unfortunate to have this full frame-tree walk... I suspect something like swithing the `visibility` of the `body` to `hidden` in a case like:

```
<style>
div { position: absolute }
</style>
<body><div><div><div><div><!-- ... -->
```

throttling it correctly would involve a _lot_ of frame-tree walking :(.

But I can't think of anything much better honestly. Also I guess given this should run relatively unfrequently this is r=me, thanks for bearing with me :)

Comment 64

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

https://reviewboard.mozilla.org/r/213032/#review223550

::: layout/style/nsStyleStruct.cpp:4106
(Diff revision 4)
>      if ((mImageOrientation != aNewData.mImageOrientation)) {
>        hint |= nsChangeHint_AllReflowHints |
>                nsChangeHint_RepaintFrame;
>      }
>      if (mVisible != aNewData.mVisible) {
> +      hint |= nsChangeHint_VisibilityChange;

May be worth to guard this in `if (mVisible == VISIBLE || aNewData.mVisible == VISIBLE)`.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Thanks for careful reviews!

(In reply to Emilio Cobos Álvarez [:emilio] from comment #63)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review223546
> 
> My last question is how does this play with frame continuations and such. I
> guess animations only look at primary frames? If so, this is fine I guess,
> but we never call `UpdateVisibilityState` on the later continuations...
> Should we?

Yeah, as far as I know, we only care the primary frame.  But as per bug 1408731, there are some cases that animations on continuations don't work properly.  Someday I have to revisit bug 1408731, and should look into in detail what's going on there.  After that I can revisit this optimization again.  (After that I think I have to audit all over the place for animations)

> ::: layout/generic/nsFrame.cpp:11379
> (Diff revision 4)
> > +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> > +         ancestor = ancestor->GetInFlowParent()) {
> > +      ancestor->mAllDescendantsAreInvisible = false;
> > +    }
> > +  } else {
> > +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);
> 
> Ok, so after thinking a bit more thoroughly, there's no great thing I can
> think of that doesn't involve depending on the style tree on the children to
> be up-to-date...
> 
> I think it's very unfortunate to have this full frame-tree walk... I suspect
> something like swithing the `visibility` of the `body` to `hidden` in a case
> like:
> 
> ```
> <style>
> div { position: absolute }
> </style>
> <body><div><div><div><div><!-- ... -->
> ```

Yes, that's right. Tons of flat children is the worst case for this optimizations.

Comment 66

[Hiroyuki Ikezoe (:hiro)]

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3558c80ad9c5294ad93253b52430ba71ae5e784f

And talos on linux64;
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=3558c80ad9c5294ad93253b52430ba71ae5e784f&framework=1&filter=linux64%20pgo&selectedTimeRange=172800

Seems fine?

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213032/diff/4-5/

Comment 68

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213034/diff/4-5/

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213036/diff/4-5/

Comment 70

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213038/diff/4-5/

Comment 71

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213040/diff/4-5/

Comment 72

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213042/diff/4-5/

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/027b0c65d944
Add VisibilityChange change hint. r=emilio
https://hg.mozilla.org/integration/autoland/rev/43ca55e7c93b
An additional check that an animation on visibility: hidden element starts restyling when the element gets visible. r=boris
https://hg.mozilla.org/integration/autoland/rev/0b9872865f0e
Test for an animation in the parent element whose visibility is changed. r=boris
https://hg.mozilla.org/integration/autoland/rev/c481f409feaa
Test for an animation on a visibility:hidden element which has a child. r=boris
https://hg.mozilla.org/integration/autoland/rev/2dbbfc331bdf
Test for an animation on visibility: hidden element which has grandchild. r=boris
https://hg.mozilla.org/integration/autoland/rev/f8d771835fd2
Throttle animations on visibility:hidden element. r=birtles,boris,emilio

Comment 74

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on /browser_toolbariconcolor_restyles.js.

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=41758580953c08c004b85f1db067264ff4f38691
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=160523758&repo=autoland&lineNumber=2854

Comment 75

[Hiroyuki Ikezoe (:hiro)]

I don't quite understand why the failure didn't appear on the try in comment 66.
One possibility I can think of is that the try was based on m-c.

Comment 76

[Hiroyuki Ikezoe (:hiro)]

Hmm, I did push another try based on m-c but it has already included today's merge.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b04839164fe455346b51593c62019ef1835047f1&selectedJob=160588043

It's green.  I did retrigger there, let's see what happens.

Comment 77

[Hiroyuki Ikezoe (:hiro)]

It seems to be an intermittent happens only on Windows. sigh.

Comment 79

[Hiroyuki Ikezoe (:hiro)]

Though I still don't understand why the test failure happens only on Windows, I noticed that when we unthrottle the transform animation in visibility:hidden we need to check the element is in a scrollable ancestor unlike the scrolled-out animation.  That's because visibility:hidden element keeps invisible even after the element is moved into view.

With this fix, the failure on Windows has gone away.  (There is an unrelated orange).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=080ec871e301bfb59eefe44fe816229a1a1e9f93

Comment 80

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1237454 - Unthrottle transform animations in visibility:hidden element periodically only if the element is scrolled out.

In the case where we throttle transform animations in visibility:hidden
element, we just need to unthrottle only if the element is scrolled out since
unlike the scrolled out element, visibility:hidden element keeps invisible
even after the element moved into view.

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

Comment 81

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213032/diff/5-6/

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213034/diff/5-6/

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213036/diff/5-6/

Comment 84

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213038/diff/5-6/

Comment 85

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213040/diff/5-6/

Comment 86

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213042/diff/5-6/

Comment 87

[Brian Birtles (:birtles)]

Comment on attachment 8949660 [details]
Bug 1237454 - Unthrottle transform animations in visibility:hidden element periodically only if the element is scrolled out.

https://reviewboard.mozilla.org/r/219008/#review225412

At some point we should add comments explaining the difference between CanThrottleTransformChanges and CanThrottleTransformChangesInScrollable.

Comment 88

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #87)
> Comment on attachment 8949660 [details]
> Bug 1237454 - Unthrottle transform animations in visibility:hidden element
> periodically only if the element is scrolled out.
> 
> https://reviewboard.mozilla.org/r/219008/#review225412
> 
> At some point we should add comments explaining the difference between
> CanThrottleTransformChanges and CanThrottleTransformChangesInScrollable.

OK, I will add comments their in a later bug (probably in bug 1437272).

I hope this will be the very final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf80d62dcc988588ea9a5b0003c3f9b49601e9f

Comment 89

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79a183e59e6c
Add VisibilityChange change hint. r=emilio
https://hg.mozilla.org/integration/autoland/rev/25517928b230
An additional check that an animation on visibility: hidden element starts restyling when the element gets visible. r=boris
https://hg.mozilla.org/integration/autoland/rev/3948591d0a91
Test for an animation in the parent element whose visibility is changed. r=boris
https://hg.mozilla.org/integration/autoland/rev/3217309b2105
Test for an animation on a visibility:hidden element which has a child. r=boris
https://hg.mozilla.org/integration/autoland/rev/2588e518b520
Test for an animation on visibility: hidden element which has grandchild. r=boris
https://hg.mozilla.org/integration/autoland/rev/d57b9784043c
Throttle animations on visibility:hidden element. r=birtles,boris,emilio
https://hg.mozilla.org/integration/autoland/rev/0743a6a82eeb
Unthrottle transform animations in visibility:hidden element periodically only if the element is scrolled out. r=birtles

Comment 90

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/79a183e59e6c
https://hg.mozilla.org/mozilla-central/rev/25517928b230
https://hg.mozilla.org/mozilla-central/rev/3948591d0a91
https://hg.mozilla.org/mozilla-central/rev/3217309b2105
https://hg.mozilla.org/mozilla-central/rev/2588e518b520
https://hg.mozilla.org/mozilla-central/rev/d57b9784043c
https://hg.mozilla.org/mozilla-central/rev/0743a6a82eeb