1419339 - IntersectionObserver doesn’t fire on 'transform' animations unless rAF

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[surma]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.89 Safari/537.36

Steps to reproduce:

1. Open https://io-raf-bug.glitch.me/ and don’t move the mouse or trigger any other form of event. The text in the blue box does not update (or at least not consistently).
2. Move the mouse frantically to see the blue box update



Code: https://glitch.com/edit/#!/io-raf-bug?path=index.html:67:0



Expected results:

Box should update without mouse movements

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

NI someone involved with IntersectionObserver to look at after the holiday.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (from reporter)

Comment 3

[Daniel Holbert [:dholbert]]

CC'ing mstange who's also reviewed some intersectionobserver code.

It looks to me like this is a case where the animation is being performed in the compositor -- and back on the main thread, we're not correctly detecting that the intersection is changing due to these operations happening over in the compositor & that we need to fire events accordingly. (Strengthening that theory: if I adjust the testcase to use relative positioning instead of transforms, and I animate the "left" property instead of transform, then the bug goes away. This makes sense, because we don't handle 'left' animations in the compositor.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 1 (animating 'left' instead of 'transform' -- gives expected results)

Comment 5

[Markus Stange [:mstange]]

That seems like the right explanation. CC'ing hiro and birtles - I know we have a workaround for updating scrollbars with some rate-limit under similar circumstances. So we may need to do something like that in the presence of intersection observers. (I don't really know how those rate-limited reflows are triggered, though.)
See also bug 1305976 which has a similar problem.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Yeah, Markus is right.  We unthrottle (synchronize to the compositor) transform animations every 200ms if the parent element has scrollbars.  Actually I see the unthrottle behavior with attachment 8931026 [details] on my environment somehow. :)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Unthrottle transform animations if there is any IntersectionObservers

Like this.  Actually this is not perfect, if we want to update IntersectionObserver more precisely we have to not run the transform animations on the compositor.

Comment 8

[Daniel Holbert [:dholbert]]

(Thanks, hiro! Clearing my needinfo, since it seems like you've got this.)

Comment 9

[Brian Birtles (:birtles)]

I wonder what the desired frequency is of updates to IntersectionObserver. Perhaps it would be enough to unthrottle every 200ms or so like we do for scrolling? It would be unfortunate if authors have to choose between good perf (i.e. not restyling every frame) or using IntersectionObserver.

(Also I think attachment 8931151 [details] [diff] [review] adds the check too late in the function? We return true earlier in the function if the platform doesn't show scrollbars meaning we'll still end up throttling on Android with this patch.)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)

> (Also I think attachment 8931151 [details] [diff] [review] adds the check
> too late in the function? We return true earlier in the function if the
> platform doesn't show scrollbars meaning we'll still end up throttling on
> Android with this patch.)

Yep, right.  I couldn't test it because I can't reproduce this bug locally.   Also for this bug, I think we need a test case similar to the test case which has been failed intermittently (bug 1413370), and I think the intermittent bug will be fixed by bug 1413817 (at least it should get more robust by that bug).  So I'd like to fix this bug after bug 1413817.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I and Brian had discussed about this and decided to land the fix without any test cases here.  And we will add test cases in later bug after bug 1193394 fixed.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I thought I did push a request review here.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1419339 - Unthrottle transform animations periodically if there is any IntersectionObservers.

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

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8948815 [details]
Bug 1419339 - Unthrottle transform animations periodically if there is any IntersectionObservers.

https://reviewboard.mozilla.org/r/218190/#review224038

::: dom/animation/KeyframeEffectReadOnly.cpp:1543
(Diff revision 1)
> +    GetRenderedDocument()->HasIntersectionObservers();
> +
>    // If we know that the animation cannot cause overflow,
>    // we can just disable flushes for this animation.
>  
> -  // If we don't show scrollbars, we don't care about overflow.
> +  // If we don't show scrollbars and have no intersection observer, we don't

Super minor nit: s/observer/observers/

::: dom/animation/KeyframeEffectReadOnly.cpp:1554
(Diff revision 1)
> +  // If we have any intersection observers, we do unthrottle this transform
> +  // animation periodically.

Another minor nit: s/we do/we/

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8948815 [details]
Bug 1419339 - Unthrottle transform animations periodically if there is any IntersectionObservers.

https://reviewboard.mozilla.org/r/218190/#review224040

::: dom/animation/KeyframeEffectReadOnly.cpp:1537
(Diff revision 1)
> +  bool hasIntersectionObservers =
> +    GetRenderedDocument()->HasIntersectionObservers();

On further thought, is there any possibility GetRenderedDocument could return null? If not, should we add a comment or assertion about this?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #15)
> Comment on attachment 8948815 [details]
> Bug 1419339 - Unthrottle transform animations periodically if there is any
> IntersectionObservers.
> 
> https://reviewboard.mozilla.org/r/218190/#review224040
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:1537
> (Diff revision 1)
> > +  bool hasIntersectionObservers =
> > +    GetRenderedDocument()->HasIntersectionObservers();
> 
> On further thought, is there any possibility GetRenderedDocument could
> return null? If not, should we add a comment or assertion about this?

Oh right, indeed.  I believe with current setup, this function is called with the target element which is associated with a document.  But I will add a check that bails out in the case, just in case.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

A finial try but it got involved with a bug (bug 1392391) that was already backed out in m-c because of jsreftest failures on Android.

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

So, here is another try on Android
https://treeherder.mozilla.org/#/jobs?repo=try&revision=39834845653346d9a9aa5587f7173a5e80fc21b7

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8948815 [details]
Bug 1419339 - Unthrottle transform animations periodically if there is any IntersectionObservers.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c149e5048e65
Unthrottle transform animations periodically if there is any IntersectionObservers. r=birtles

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c149e5048e65

Comment 21

[Stefania]

I managed to reproduce the bug using an older version of  Nightly (2018-02-21) on Windows 10 x64.
I retested everything on latest Nightly 61.0a1 (20180107100322) and beta 60.0b4 using Windows 10 x64, macOS 10.13 and Ubuntu 16.04 and the bug is not reproducing anymore.