Closed Bug 1409166 Opened 7 years ago Closed 7 years ago

Throttle animations which is scrolled out from an ancestor element but is in-view in the parent

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Platform:
Unspecified
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: yoasif, Assigned: hiro)

References

(
URL
)

Details

(Keywords: nightly-community)

Attachments

(2 files)

Simplified test case
662 bytes, text/html
Details
Bug 1409166 - Check whether the target animating frame is scrolled out of each scrollable ancestor.
59 bytes, text/x-review-board-request
birtles
: review+
mattwoodrow
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171016100113

Steps to reproduce:

1. Navigate to: https://uk.reuters.com/article/uk-unesco-vote/unesco-selects-frances-azoulay-as-new-chief-idUKKBN1CI15X
2. Wait until page is finished loading


Actual results:

Page continues to take up 50% CPU, causing fans to spin up (especially when other tabs are loaded). 


Expected results:

Minimal CPU use, fans should not kick in. 

Performance profile: https://perfht.ml/2ysPWZi

I'm running the latest Nightly on macOS.
Still an issue on the latest Nightly with a completely fresh profile.
Would disabling layout.css.servo.enabled make things better?
No improvement in CPU use, in fact it may be a bit worse (55-70%).

CPU fans are not spinning up, but that may be simply because the ambient temperature is lower.
> status-firefox58: --- → unaffected
Is this a WebRender issue (or why do you think it won't reach Beta/Release)?
Keywords: nightly-community
OS: Unspecified → Mac OS X
Version: 58 Branch → Trunk
Oops, I filled those in incorrectly; not running WebRender on macOS at this time. Corrected.
status-firefox56: ? → ---
status-firefox57: ? → ---
See Also: → 1404042
Is this a regression from earlier Firefox versions, or did it start as a result of a change to reuters.com?  If the former, somebody should use mozregression or similar tools to figure out when it regressed.
Just tried this on v55, and it is about as bad as it is on the latest Nightlies.
I am seeing three opacity animations on the site, two of them are being throttled because they are out of view, but the last one is not, I think it is consuming CPU.  I will debug it later.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> I am seeing three opacity animations on the site, two of them are being
> throttled because they are out of view, but the last one is not

Correction: All three animations is *NOT* being throttled. I might be seeing other animations, though I couldn't see any other animations on the site.
Attached file Simplified test case — 
The animations are inside a div which has sufficient height to view the animations, say 2000px, but the div is inside another div which has insufficient height to view whole the child div.  So nsIFrame::IsScrolledOutOfView fails to check the animating frame is viewable or not.
Flags: needinfo?(hikezoe)
Component: General → DOM: Animation
See Also: → 1166500, 1315465
What we need to do here is to check whether the animating frame is scrolled out of each ancestors.  We are currently check each scrollable parent is scrolled out of its scrollable ancestor.

https://hg.mozilla.org/mozilla-central/file/64bab5cbb9b6/layout/generic/nsFrame.cpp#l10792

  nsRect rect = aFrame->GetVisualOverflowRectRelativeToSelf();                  
                                                                                
  nsRect transformedRect =                                                      
    nsLayoutUtils::TransformFrameRectToAncestor(aFrame,                         
                                                rect,                           
                                                scrollableParent);
Comment on attachment 8922184 [details]
Bug 1409166 - Check whether the target animating frame is scrolled out of each scrollable ancestor.

https://reviewboard.mozilla.org/r/193218/#review198470

I think this makes sense but I'm not familiar with this code so I hope Matt is.

::: dom/animation/test/chrome/test_restyles.html:328
(Diff revision 2)
>        { style: 'overflow-y: scroll; height: 100px;' });
>      var div = addDiv(null,
> -      { style: 'animation: background-color 100s; position: relative; top: 100px;' });
> +      { style: 'animation: background-color 100s; ' +
> +               'position: relative; ' +
> +               'top: 60px;' }); // This element is in-view in the parent, but
> +                                // out of view in the grand parent.

(Very very minor nit: grandparent)
Attachment #8922184 - Flags: review?(bbirtles) → review+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8922184 [details]
Bug 1409166 - Check whether the target animating frame is scrolled out of each scrollable ancestor.

https://reviewboard.mozilla.org/r/193218/#review198486
Attachment #8922184 - Flags: review?(matt.woodrow) → review+
Thank you!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e760609ee0bb
Check whether the target animating frame is scrolled out of each scrollable ancestor. r=birtles,mattwoodrow
Hiro, should we uplift this animation fix to Beta 57? Asif said in comment 8 that this problem is reproducible in Firefox 55. If this is not a regression, then perhaps we can live with this bug in 57. :)
status-firefox56: --- → wontfix
status-firefox57: --- → ?
Flags: needinfo?(hikezoe)
Yes, right.  This is not a regression at all.  People will not complain "hey, Firefox 57 is much slower than before" in regard to this bug. :)
status-firefox57: ? → wontfix
Flags: needinfo?(hikezoe)
https://hg.mozilla.org/mozilla-central/rev/e760609ee0bb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Changing summery to reflect what we did in this bug.
Summary: High CPU use on Reuters article page on MBP 2015 → Throttle animations which is scrolled out from an ancestor element but is in-view in the parent
Depends on: 1412535
No longer depends on: 1412535
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: