1052109 - No way to reliably wait for scrollIntoView() to complete due to async scrolling

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is a common pattern in devtools tests as scrolling in async and we need to wait for a node to be scrolled into view in able to interact with it using the mouse:
cbx.scrollIntoView();
executeSoon(() => {
  EventUtils.synthesizeMouseAtCenter(cbx, {}, cbx.ownerGlobal);
});

It seems that now we are adapting our tests for e10s we can't rely on just using executeSoon()... in fact, in one place we need to use:
cbx.scrollIntoView();
executeSoon(() => {
  executeSoon(() => {
    executeSoon(() => {
      EventUtils.synthesizeMouseAtCenter(cbx, {}, cbx.ownerGlobal);
    });
  });
});

This is ugly and still unreliable... can we not emit a "scrollIntoView" event at the end of PresShell::ScrollContentIntoView()? A "scroll" event doesn't work because it is emitted too early.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems less than ideal that the scroll event fires before the scrolling has happened.

(I'm assuming the async scrolling you're talking about is not smooth scrolling, but scrolling that happens all at once, just asynchronously.)

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #1)
> It seems less than ideal that the scroll event fires before the scrolling
> has happened.
>

Yes, I suppose the real problem is that the scroll event appears to be fired before the document has finished scrolling, or at least it appears that this is true as we then still need executeSoon().
 
> (I'm assuming the async scrolling you're talking about is not smooth
> scrolling, but scrolling that happens all at once, just asynchronously.)

If that is what element.scrollIntoView() uses then yes.

Comment 3

[Chris Peterson [:cpeterson]]

These DevTools bugs should probably block e10s from riding to Aurora.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Moving to Core::DOM Events

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Nothing to do with DOM Events implementation. It is up to the code which dispatches the event to deal with this stuff. So, layout.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #0)
> It seems that now we are adapting our tests for e10s we can't rely on just
> using executeSoon()... in fact, in one place we need to use:
> cbx.scrollIntoView();
> executeSoon(() => {
>   executeSoon(() => {
>     executeSoon(() => {
>       EventUtils.synthesizeMouseAtCenter(cbx, {}, cbx.ownerGlobal);
>     });
>   });
> });
I thought scroll event was supposed to fire during the next animation frame tick.
So there can be arbitrary number of event loop ticks before that happens and executeSoon won't help there. 
You want requestAnimationFrame (and perhaps inside it one executeSoon)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Though, if 'scroll' event happens too early, that is still a bug, in layout ;)

Comment 8

[Jim Mathies [:jimm]]

dev tools test bugs are being dealt with by the dev tools team so they don't block e10s milestones.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Kats, you've added some things to address this, right?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Mike, is this still a problem? If it is, can you try making sure the test.events.async.enabled pref is set to false? That might help, if my understanding of the problem is correct. The scrollIntoView call should be synchronous AFAICT, unless you tell it to be smooth by passing in the relevant options - the async'ness probably comes from the delay in propagating that scroll offset to the APZ code, and synthesizeMouseAtCenter getting routed through APZ before that happens. Flipping the pref I mentioned should make avoid the event from getting routed through APZ.

If that doesn't help, pointing me to a specific test (ideally reduced as much as possible) that demonstrates the behaviour would be useful so I can figure out what's going on.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Mike, is this still a problem? If it is, can you try making sure the
> test.events.async.enabled pref is set to false? That might help, if my
> understanding of the problem is correct. The scrollIntoView call should be
> synchronous AFAICT, unless you tell it to be smooth by passing in the
> relevant options - the async'ness probably comes from the delay in
> propagating that scroll offset to the APZ code, and synthesizeMouseAtCenter
> getting routed through APZ before that happens. Flipping the pref I
> mentioned should make avoid the event from getting routed through APZ.
> 
> If that doesn't help, pointing me to a specific test (ideally reduced as
> much as possible) that demonstrates the behaviour would be useful so I can
> figure out what's going on.

I haven't seen this recently... we can close it and create a new bug if it comes up again.

Comment 12

[Marco Chen [:mchen]]

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #1)
> (I'm assuming the async scrolling you're talking about is not smooth
> scrolling, but scrolling that happens all at once, just asynchronously.)

Hi David,

If it is with option of smooth then I found I would receive multiple times of scroll events. Then how do I decide whether scrollIntoView is done or not? Thanks.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do other browser engines have the same behavior?  If so, maybe there's a need to standardize an event that scrolling has completed?  Or have some other engines already implemented such an event?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Bug 1172171 has a proposal for a "scrollend" event. AIUI all browsers have this problem, so it might be worth standardizing something here.