1657627 - Mouse and scroll position disagree on charcod.es

Status: VERIFIED FIXED

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

Description

[M8R-p7]

Attachment: Video of 'Actual Results'

STR

0. Create a fresh profile
1. Visit https://charcod.es/#letter_like
2. Scroll down and click a symbol in the last line. Information about the symbol will appear.
3. If necessary, scroll to the very bottom again.
4. Click another symbol in the last few lines.

Expected Results:
When clicking a symbol in the last line, the scroll position should jump to bring the information panel to the bottom of the screen. Selecting other symbols should work as expected.

Actual Results:
The scroll position is not visually reset, but the page behaves as if it were - the mouse 'hover' activates items far above the cursor's actual position. This effect persists even with the DevTools Inspector as shown in the accompanying video.

Regression Range:
mozregression thinks this is regressed by bug1644271 (good: 61fccfec51e98f51d9865bf1ff5808016a948a71, bad:01a1753056cfcc36972d43791d0f6fe43a502480), which seems possible -- in Release 79 and DevEd 80 the problem is not present when the preference apz.mvm.force-enabled is set to 'false'. In Nightly 81 the issue always occurs whatever the preference. Tested on Win10 and Linux.

Comment 1

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

Thanks for filing! I can investigate.

Comment 2

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

Hm, looks like the main thread and compositor disagree about the scroll position. In the good version (prior to bug 1644271) clicking on letters also adjusts the scroll position so that the info box is at the bottom of the window. But after bug 1644271 it seems like the main thread scroll position gets updated while the compositor scroll position does not, resulting in the visual mismatch. And then doing some scrolling resyncs the compositor scroll position back to the main thread and things are good again.

Comment 3

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

Attachment: Reduced test case

The content height shrinks and expands within a single vsync interval, but also forces the main-thread scroll position to be clamped to the reduced content height. After that is where things go out of sync. Attaching reduced testcase.

Comment 4

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

We seem to be hitting this codepath which prevents the main thread from clamping the APZ visual scroll position, which is why they end up out of sync. The comment above that block refers to bug 1543485 implementing a proper fix, and Botond is working on that. The fix for that bug may fix this issue too, or at least might unlock the fix for this. I'll think about this a bit more and see if there's a way to fix it in a localized way.

Comment 5

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

Discussed this with Botond and we can up with a simple fix for uplifting to beta; we can replace the GetIsViewportOverridden check for an apz.allow_zooming check. That will retain the originally intended behaviour of restricting the codepath to mobile. It won't fix it on desktop nightly though, since we have that pref turned on there - but Botond intends to fix bug 1543485 for the 81 nightly cycle and after that we should be able to fix this by removing the codepath entirely.

Comment 6

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

Here's a try push with that change and apz.allow_zooming turned to false, to ensure that this change won't break tests when uplifted to beta.
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=5cfd73c8c6a5667750792a0e236db1fa1e9e359b

Comment 7

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

Attachment: Bug 1657627 - Refine condition used in temporary workaround code. r?botond

This restricts the workaround to mobile a bit better, since the
GetIsViewportOverridden() check returns true on desktop as of bug 1644271.

Comment 8

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

I intend to land this patch on 81 nightly and uplift to 80 beta. That way if we don't fix bug 1543485 in the nightly 81 cycle at least this patch will ride the trains to 81 beta, and we won't need additional work to prevent the bug from manifesting in 81 release.

Marking the bug leave-open since the patch won't fix it in nightly yet.

Comment 9

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

Comment on attachment 9168882 [details]
Bug 1657627 - Refine condition used in temporary workaround code. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: On some pages, it's possible to get into a state where effects based on mouse position are visually offset (see STR/video on this bug and bug 1650977). This state persists until the user scrolls the page.

Note this patch doesn't fix the issue on Nightly because Nightly has the apz.allow_zooming pref set to true. But the patch will fix the issue on Beta while we work on the more involved fix for Nightly.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR in comment 0, or use the reduced test case attached to the bug. Again - note that this patch landing on Nightly will have no effect, please only test on the beta build that contains the patch after uplift.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple condition swap from one that used to be true on Android only but no longer is, to one that is still true on Android only (on beta/release).
• String changes made/needed: none

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/528cf50c8b8f
Refine condition used in temporary workaround code. r=botond

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/528cf50c8b8f

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9168882 [details]
Bug 1657627 - Refine condition used in temporary workaround code. r?botond

approved for 80.0b7

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2afb0bc8c00d

Comment 14

[Alin Ilea, Desktop Test Engineering]

Reproduced the issue using a Nightly build 81.0a1 (2020-08-11) on Windows 10.
Verified - Fixed in latest Beta 80.0b7 (build id: 20200810131809) using the steps from the description and verified also with the reduced test case attached on Windows 10 and Ubuntu 18.04.

Comment 15

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

I'm going to use this bug as the canonical version since there's a bunch of bugs filed now that are effectively the same root cause.

Comment 19

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

Verified that this is fixed in the latest nightly thanks to bug 1543485. I'll use this bug to land a test.

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=0864f94c52575cc1fffc19bf1c9014624e400002

Comment 21

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

Attachment: Bug 1657627 - Add a test. r?botond

Comment 22

[Jim Mathies [:jimm]]

Apparently fixed by bug 1543485 in 81.

Comment 23

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

The uplift fixed from this bug (comment 13) fixed it in 80, not sure why it was marked "verified disabled". And bug 1543485 fixed it in 82 as well, so if we're marking 81 as fixed, we should mark 82 as fixed too.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff12ca4b41aa
Add a test. r=botond

Comment 25

[Mathew Hodson]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)

The uplift fixed from this bug (comment 13) fixed it in 80, not sure why it was marked "verified disabled". And bug 1543485 fixed it in 82 as well, so if we're marking 81 as fixed, we should mark 82 as fixed too.

In Firefox 80, the code path with the issue is disabled when the pref apz.allow_zooming is false, which it is for Release but not Nightly. I think it makes sense to have a different status, because the patch that was uplifted wouldn't fix the issue on desktop Nightly. Instead, it disables it for Release.

Comment 26

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ff12ca4b41aa

Comment 27

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

(In reply to Mathew Hodson from comment #25)

In Firefox 80, the code path with the issue is disabled when the pref apz.allow_zooming is false, which it is for Release but not Nightly. I think it makes sense to have a different status, because the patch that was uplifted wouldn't fix the issue on desktop Nightly. Instead, it disables it for Release.

Ah, I see. That's a reasonable argument since the semantics of these flags aren't clearly defined anyway.

Comment 28

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 81.0b6 and on Firefox Nightly 82.0a1 (2020-09-03) on Windows 10 x64, Ubuntu 20.04 x64 and on MacOS 10.14.

Comment 29

[Martin Thomson [:mt:]]

I just had this issue regress on Nightly 82.0a1 (2020-09-13). It was the list of rooms on the Mozilla slack. See also bug 1664838.