1874551 - The JavaScript function getBoundingClientRect() returns prematurely, while the final position is still being calculated.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[incg]

Attachment: Simple.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:121.0) Gecko/20100101 Firefox/121.0

Steps to reproduce:

Opened the attached HTML, clicked on the clickable element.

Actual results:

Clicking on the clickable element often moves it away, even if there is enough space above and below it.

Expected results:

Clicking on the clickable element should not alter its position, space allowing. Specifically, getBoundingClientRect() should wait for the layout calculation to finish. As it is now (121.0.1 64-Bit), the result is of no use. (BTW: scrollIntoView() works fine; I use it for a makeshift workaround.)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Text and Fonts' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Mayank Bansal]

Thanks for reporting!

Regressed by :
Bug 1856088 - Handle scroll anchor adjustments as an absolute scroll position updates behind a pref. r=botond

Differential Revision: https://phabricator.services.mozilla.com/D190888

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1856088

:hiro, since you are the author of the regressor, bug 1856088, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Can you please attach screen recordings? I don't see the issue at all. Given that scrollIntoView() works, I suspect the move distance is 1px? Then it's due to our scroll position rounding issues.

Comment 5

[Alice0775 White]

I can reproduce the issue.
The move distance is depended on the initial scroll position.

See screen cast: https://youtu.be/z9XyPcC9uas

Comment 6

[Emilio Cobos Álvarez (:emilio)]

See above :)

Comment 7

[incg]

Alice did it for me, thanks.

In the debugger all works fine, since the breakpoint allows for enough time for the layout recalculation to complete. In other cases the deviation can be very large. I tried several callbacks, of which only one worked reliably, but caused ugly flickering, because it arrived only after the layout was not only recalculated, but actually displayed. I devised a makeshift with scrollIntoView (applied to an invisible dummy element) which works correctly without flickering, but may not do so in other settings.

I also asked in vain for StackOverflow experts to comment, since I was not sure whether it is a bug or a feature. It now seems most reasonable to me that all queries for viewport positions should wait until the current layout changes are calculated, but not yet painted. If I (or you) were Mr. JavaScript, I might propose more flexible language constructs or callbacks.

Comment 8

[incg]

Alice did it for me, thanks.

In the debugger all works fine, since the breakpoint allows for enough time for the layout recalculation to complete. In other cases the deviation can be very large. I tried several callbacks, of which only one worked reliably, but caused ugly flickering, because it arrived only after the layout was not only recalculated, but actually displayed. I devised a makeshift with scrollIntoView (applied to an invisible dummy element) which works correctly without flickering, but may not do so in other settings.

I also asked in vain for StackOverflow experts to comment, since I was not sure whether it is a bug or a feature. It now seems most reasonable to me that all queries for viewport positions should wait until the current layout changes are calculated, but not yet painted. If I (or you) were Mr. JavaScript, I might propose more flexible language constructs or callbacks.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Thank you, Alice. It looks like a key factor to reproduce this issue on my environment is to position the clickable element at the top half of the page for some reasons.

The reason of the jump is we don't set mApzScrollPos in the case of scroll anchoring, thus the subsequent scrollBy call reaches to a wrong position.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1874551 - Set `mApzScrollPos` in the case of `ScrollOrigin::AnchorAdjustment`. r?botond

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c8bee57b7fe
Set `mApzScrollPos` in the case of `ScrollOrigin::AnchorAdjustment`. r=botond

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44022 for changes under testing/web-platform/tests

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for scroll related wpt failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=3c8bee57b7fe118bb33346504f6d16610d749170&tochange=4e22ff9aca62091c4fe90ac7d83dbcd3e6fec910&searchStr=wpt&selectedTaskRun=bB2dM_JoRIKyyClvkkpiKQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=443529434&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/4e22ff9aca62091c4fe90ac7d83dbcd3e6fec910

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Gosh, it's the 1px rounding issue. The test was passed on my last try push here. :/

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 16

[Hiroyuki Ikezoe (:hiro)]

With layout.scroll.disable-pixel-alignment=true, I can constantly see 0.5px difference failure in the test locally, so I tracked down. The root cause is bug 1875011.

In the case where the window.innerHeight is an odd number scrollIntoView({ block: "center" }) will be positioned at a fractional scroll position, with layout.scroll.disable-pixel-alignment=true the fractional scroll position is preserved. So far so good. But because of bug 1875011, scrollBy call (even with integer deltas) rounds the fractional position thus it will reach to a wrong position where it should be.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/875fd9f694c9
Set `mApzScrollPos` in the case of `ScrollOrigin::AnchorAdjustment`. r=botond

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/875fd9f694c9

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9372758 [details]
Bug 1874551 - Set mApzScrollPos in the case of ScrollOrigin::AnchorAdjustment. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: scroll position jumps unexpectedly
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple, just a one line change it will never cause any critical issues.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9372758 [details]
Bug 1874551 - Set mApzScrollPos in the case of ScrollOrigin::AnchorAdjustment. r?botond

Fx122 is now in release, switching uplift from beta to release.
This can be considered as a Fx122 dot release ride-along.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)

• List of other uplifts needed: None

Oops this is wrong. Just D198512 is only one patch to fix this bug, but its the patch includes a mochitest which will be flaky without bug 1875011. So for bug 1875011, it's basically just renaming internal functions and using a slightly different version of a function to fix the flaky test.

Comment 24

[Donal Meehan [:dmeehan]]

Comment on attachment 9372758 [details]
Bug 1874551 - Set mApzScrollPos in the case of ScrollOrigin::AnchorAdjustment. r?botond

Approved for 122.0.1

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/c1ef575b0639