Closed Bug 1419339 Opened 4 years ago Closed 4 years ago

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

Categories

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

56 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: surma, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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
Component: Untriaged → DOM
Product: Firefox → Core
NI someone involved with IntersectionObserver to look at after the holiday.
Flags: needinfo?(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.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: IntersectionObserver doesn’t fire on animations unless rAF → IntersectionObserver doesn’t fire on 'transform' animations unless rAF
Flags: needinfo?(dholbert)
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.
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. :)
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.
Assignee: nobody → hikezoe
(Thanks, hiro! Clearing my needinfo, since it seems like you've got this.)
Flags: needinfo?(dholbert)
Attachment #8931151 - Attachment is patch: true
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.)
(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.
Priority: -- → P2
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
Blocks: 1434146
I thought I did push a request review here.
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/
Attachment #8948815 - Flags: review?(bbirtles) → review+
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?
(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.
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
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c149e5048e65
Unthrottle transform animations periodically if there is any IntersectionObservers. r=birtles
https://hg.mozilla.org/mozilla-central/rev/c149e5048e65
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
QA Whiteboard: [good first verify]
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.
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.