Bug 138873 – Add layout test for DOM timers throttling and 0 height element with visible overflowing content

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Add a layout test for verify that a DOM timer changing the style of a 0 height element with visible overflowing content does not get throttled. If the implementation does not properly use the overflow rect to determine if the element is visible, this test would fail. This covers the top scrolling banner on huffingtonpost.com.

[Chris Dumez]

Created attachment 241864 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 241864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241864&action=review

> LayoutTests/fast/dom/repeating-timer-element-overflow-throttling.html:4
> +<ul id="testElement" style="height: 0; overflow: visible;">

overflow: visible is the default.

[Simon Fraser (smfr)]

What about opacity:0 or visibility:hidden elements? What if the timer toggles the size of the element to/from 0? What if the timer does other useful work other than touch style? If a timer touches several elements, only some of which are outside the viewport, does it get throttled?

What if the page uses requestAnimationFrame to touch style on hidden elements?

[Chris Dumez]

(In reply to comment #3)
> What about opacity:0 or visibility:hidden elements? 

I will have to check. If it returns in the element having no renderer (like 'display: none'), it would cause the timer to get throttled. If they have a renderer, we may need to add code for handling this to try and throttle timers that change the style of the element (as they are not visible to the user). Likely, such timers don't get throttled right now, but this is safe and can be improved later.

> What if the timer
> toggles the size of the element to/from 0?

It should cause layout, right? and on layout we will re-evaluate if the elements causing the timer to be throttled is still not visible.

> What if the timer does other
> useful work other than touch style?

Yes, this is the risk. If we find such cases, we could add more metrics to detect if a timer has visible side-effects. We have been throttling timers that interact with non-visible plugins for a while now, and so far, this hasn't been an issue.

> If a timer touches several elements,
> only some of which are outside the viewport, does it get throttled?

No, if the timer touches the style of *any* element inside the viewport, we won't throttle it. I will add test coverage for this.

> What if the page uses requestAnimationFrame to touch style on hidden
> elements?

Then it is not using DOMTimers (AFAIK), and thus won't be affected by throttling.

[Chris Dumez]

Created attachment 241865 [details]
Patch

[Simon Fraser (smfr)]

(In reply to comment #4)

> > What if the page uses requestAnimationFrame to touch style on hidden
> > elements?
> 
> Then it is not using DOMTimers (AFAIK), and thus won't be affected by
> throttling.

rAF is increasingly being used for very CPU-heave ads (e.g. those generated by Swiffy), so we should consider throttling here, particularly when the content is inside an iframe.

Can you add a test for timer throttling inside an iframe that's scrolled out of view? That's very common.

[Chris Dumez]

> > What if the timer does other
> > useful work other than touch style?
> 
> Yes, this is the risk. If we find such cases, we could add more metrics to
> detect if a timer has visible side-effects. We have been throttling timers
> that interact with non-visible plugins for a while now, and so far, this
> hasn't been an issue.

Also note that we don't throttle timers that alter the DOM tree for this reason as well.

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #4)
> 
> > > What if the page uses requestAnimationFrame to touch style on hidden
> > > elements?
> > 
> > Then it is not using DOMTimers (AFAIK), and thus won't be affected by
> > throttling.
> 
> rAF is increasingly being used for very CPU-heave ads (e.g. those generated
> by Swiffy), so we should consider throttling here, particularly when the
> content is inside an iframe.

I can look at rAF later, see if we can apply a similar policy there. Right now, throttling DOM timers seems risky enough :) I think it is best not to extend this approach too much until we get sufficient living-on testing of this, to make sure this does not break content.

> Can you add a test for timer throttling inside an iframe that's scrolled out
> of view? That's very common.

Yes, sounds like a good thing to test indeed.

[WebKit Commit Bot]

Comment on attachment 241865 [details]
Patch

Clearing flags on attachment: 241865

Committed r176323: <http://trac.webkit.org/changeset/176323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.