Closed Bug 1662379 Opened 4 years ago Closed 3 years ago

Scroll position incorrect/stuck after reparenting scrollable element

Categories

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

79 Branch
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox80 --- wontfix
firefox81 --- wontfix
firefox82 --- wontfix
firefox83 --- wontfix
firefox87 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 --- fixed

People

(Reporter: brzezbugzilla, Assigned: kats)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.

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

Status: UNCONFIRMED → NEW
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Regressed by: 1643042
Has Regression Range: --- → yes
Assignee: nobody → kats
Attached file Standalone test case

Whoops, uploaded the wrong file last time.

Attachment #9173433 - Attachment is obsolete: true
Severity: -- → S3
Priority: -- → P2

The patches from bug 1662013 fix this issue.

Depends on: 1662013

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

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.

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.

Pushed by kgupta@mozilla.staktrace.com:
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/369922cc5c4e
1846935: apply code formatting via Lando
You need to log in before you can comment on or make changes to this bug.