Closed Bug 1052109 Opened 10 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
e10s + ---

People

(Reporter: miker, Unassigned)

References

Details

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.
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.)
(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.
These DevTools bugs should probably block e10s from riding to Aurora.
tracking-e10s: --- → m6+
Moving to Core::DOM Events
Component: Layout → DOM: Events
Nothing to do with DOM Events implementation. It is up to the code which dispatches the event to deal with this stuff. So, layout.
Component: DOM: Events → Layout
(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)
Though, if 'scroll' event happens too early, that is still a bug, in layout ;)
dev tools test bugs are being dealt with by the dev tools team so they don't block e10s milestones.
tracking-e10s: m6+ → +
Kats, you've added some things to address this, right?
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
(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.
Flags: needinfo?(dbaron)
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?
Flags: needinfo?(dbaron)
Bug 1172171 has a proposal for a "scrollend" event. AIUI all browsers have this problem, so it might be worth standardizing something here.
You need to log in before you can comment on or make changes to this bug.