1566783 - anchor jumps to incorrect position if the link is opened in foreground tab

Status: VERIFIED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Alice0775 White]

Attachment: anchor jumps to incorrect position if the link is opened in foreground tab

• setting layout.css.scroll-anchoring.enabled=false fixes the issue.
• Bug 1546027 does not fix this issue.

Reproducible: always

Steps to reproduce:

1. Load https://bug1546027.bmoattachments.org/attachment.cgi?id=9059735#anc2
2. Ctrl+Shift+Click a link( e.g "Beta") to open in foreground new tab
   --- observe scroll position
3. Ctrl+Click a link( e.g "Beta") to open in background new tab
   --- observe scroll position

Actual Results:
anchor jumps to incorrect position if the link is opened in foreground tab.
i.e, the image is displayed partially.

Expected results:
anchor jumps should be correct position even if the link is opened in foreground tab.
i.e, the whole image should be displayed.

Comment 1

[Alice0775 White]

Oops, sorry wrong STR,

1. Load https://bug1546027.bmoattachments.org/attachment.cgi?id=9059735#anc2
   instead

1. Load attachment 9078652 [details]

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Thanks for filing Alice :)

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1566783 - Don't prevent PresShell::ScrollToAnchor() from working due to scroll anchoring adjustments that happen without the user scrolling. r=dholbert

We were bailing out because scroll anchoring adjustments can make this check
fail:

• https://searchfox.org/mozilla-central/rev/22b330ecb3edba1536a54887060cbdd09db21c59/layout/base/PresShell.cpp#3194

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1566783 - Make sure to clear mLastAnchorScrolledTo after calling PresShell::ScrollToAnchor(). r=dholbert

Seems we can leave this node alive for too long if the user scrolls between
domcontentloaded (where GoToAnchor is called) and onload (where ScrollToAnchor()
is called).

Though it seems we can leave it for too long if we don't end up calling
ScrollToAnchor(), the documentation of the method claims that it's cleared
unconditionally.

Depends on D38397

Comment 5

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

:emilio, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/413557ff3269
Don't prevent PresShell::ScrollToAnchor() from working due to scroll anchoring adjustments that happen without the user scrolling. r=dholbert

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af346beee1df
Make sure to clear mLastAnchorScrolledTo after calling PresShell::ScrollToAnchor(). r=dholbert

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/413557ff3269
https://hg.mozilla.org/mozilla-central/rev/af346beee1df

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9078846 [details]
Bug 1566783 - Don't prevent PresShell::ScrollToAnchor() from working due to scroll anchoring adjustments that happen without the user scrolling. r=dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Scroll to anchor fails when something changes above the anchor node before onload.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0 / comment 1.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): One liner that fixes a scroll anchoring regression that has a couple dupes.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Scroll anchoring regression.
• User impact if declined: See above
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String or UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9078846 [details]
Bug 1566783 - Don't prevent PresShell::ScrollToAnchor() from working due to scroll anchoring adjustments that happen without the user scrolling. r=dholbert

Fixes a scroll anchoring regression which has picked up some duplicate reports. Includes an automated test. Approved for 69.0b9 and 68.1esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed74dcb0d208
https://hg.mozilla.org/releases/mozilla-beta/rev/56362f37f5bd

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/857d13bed339
https://hg.mozilla.org/releases/mozilla-esr68/rev/08cc16b32351

Comment 13

[Anca Soncutean, Desktop QA]

I’ve reproduced this issue with Fx 70.0a1 (2019-07-17) on Windows 10 x64.
The issue is verified fixed with Fx 70.0a1 (2019-07-29), Fx 69.0b9 (20190730004747) and Fx 68.1.0esr (20190729203625) on Windows 10 x64, macOs 10.14 and Ubuntu 18.04 x64.