1662379 - Scroll position incorrect/stuck after reparenting scrollable element

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Bartlomiej Brzezianski]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

1. Scroll down to the very bottom of the scrollable element you're going to reparent.
2. Reparent the element.
   3a. Try clicking on the children of the reparented container and observe which elements triggered click events.
   3b. Try scrolling down with the mousewheel on the scrollable, reparented container.

Actual results:

4a. Click handler is tiggered on a different element than currently visible and the one user clicked on.
4b. Can't scroll down.

Expected results:

5a. Click handler should have been triggered on the element the user has clicked on.
5b. Users should be able to scroll down.

Managed to reproduce on Firefox versions 79, 80, 81 in Windows 10. Also reproduced in Windows , Browserstack.

Seems like internally the scrollable element scrolls to the top on reparenting, and
a) The container is not redrawn, and the Browser visually still presents the old scroll position.
b) Sometimes the content does get redrawn, but the scrollbar is stuck in a botton position and you're unable to scroll down, unless you scroll up first.

Codepen:
https://codepen.io/barbrzez/pen/dyMzqLG

Steps to reproduce in codepen: scroll down on the LHS element, click 'Reparent' button. Try clicking on an element in reparented container. An alert should be triggered with text from the element clicked.

It's either incorrect, or if it's correct - you may not be able to scroll down with mouse wheel.

Comment 1

[Bartlomiej Brzezianski]

Also, I cannot reproduce it when Developer Tools are open.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=78301c2498bf22f4f19ae78a4e07ed9861e0931b&tochange=cc8881b3128c07537ad0c9c0a0dd35b0d9ce8a99

Comment 3

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

Attachment: Standalone test case

Comment 4

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

Attachment: Standalone test case

Whoops, uploaded the wrong file last time.

Comment 5

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

The patches from bug 1662013 fix this issue.

Comment 6

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

This is fixed in Nightly now, but I'll use this bug to land a test for the scenario.

Comment 7

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

Finally getting around to this. I rolled back my m-c clone to before bug 1662013 landed, and wrote the test, verifying that it fails (locally):
https://github.com/staktrace/gecko-dev/commits/test_bug1662379
And that it passes after rebasing it on top of bug 1662013 (again, locally):
https://github.com/staktrace/gecko-dev/commits/test_bug1662379-fixed

Now I'm gonna rebase it back to master and ensure it passes reliably.

Comment 8

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

https://treeherder.mozilla.org/jobs?repo=try&revision=769c5f25f7b16efdfa90cb51fd9de9caca137413&group_state=expanded

Comment 9

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

Attachment: Bug 1662379 - Listen for events in bubble phase rather than capture phase. r?botond

The events being listened for here are mouse events, all of which
go through both the bubble and capture phases. It's not clear to
me why I originally added this listener in the capture phase; it
is more useful in the bubble phase because then it resolves the
promise after the event has triggered listeners registered on the
target element, rather than before.

Note also that prior to the promis-ification of this function, the
callback was called inside a setTimeout(0) wrapper, which would
automatically have deferred the callback to after the event dispatch
was completed. So technically the promis-ification of this function
introduced a functional change because now the promise can get
resolved before the event dispatch is complete, and in particular,
before listeners on the target element get run. This didn't affect
anything in practice because none of the existing callers actually
have such a listener on the target element. The next patch adds
one though, and exposed this behaviour difference when I rebased it
across the promis-ification change..

This patch removes the capture:true listener option, causing the
listener to get registered in the bubble phase.

Comment 10

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

Attachment: Bug 1662379 - Add test. r?botond

Depends on D112500

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8320808e8552
Listen for events in bubble phase rather than capture phase. r=botond
https://hg.mozilla.org/integration/autoland/rev/74717197a43a
Add test. r=botond

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8320808e8552
https://hg.mozilla.org/mozilla-central/rev/74717197a43a

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/369922cc5c4e
1846935: apply code formatting via Lando

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/369922cc5c4e