2036292 - Properly use dynamictoolbar aware visual viewport size for scrollIntoView

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

This bug will fix bug 2028072.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2036292 - Make PresShell::GetVisualViewportSize aware of the dynamic toolbar height. r?botond

Previously, GetVisualViewportSize() returned the raw mVisualViewportSize, so
callers via ScrollContainerFrame::GetVisualViewportSize() were unaware of the
dynamic toolbar state. The toolbar awareness logic that was inline in
VisualViewport.cpp is moved into GetVisualViewportSize() itself, so all callers
get the correct size when the toolbar is collapsing or collapsed.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2036292 - Skip calling ScrollToVisual if the target frame is already in the visual viewport. r?botond

When scrollIntoView() is called on a position:fixed element that is already
visible in the visual viewport, ScrollFrameIntoVisualViewport() was still
calling ScrollToVisual() unnecessarily, causing a spurious visual scroll.

Add a check to skip the visual scroll if the element's rect (converted to
layout coordinates) is already contained within the current visual viewport.
Using layout coordinates is necessary since the visual viewport may have been
panned from the origin, in which case a viewport-origin check would give a
false negative for elements positioned beyond the visual viewport height.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dcf39df52d88
https://hg.mozilla.org/integration/autoland/rev/f245a2481a91
Make PresShell::GetVisualViewportSize aware of the dynamic toolbar height. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/b215f7c48f8e
https://hg.mozilla.org/integration/autoland/rev/0d02fcfe6f39
Skip calling ScrollToVisual if the target frame is already in the visual viewport. r=botond,geckoview-reviewers,layout-reviewers,emilio,m_kato

Comment 4

[rperta]

https://hg.mozilla.org/mozilla-central/rev/f245a2481a91
https://hg.mozilla.org/mozilla-central/rev/0d02fcfe6f39

Comment 5

[BugBot [:suhaib / :marco]]

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.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox152 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2036292 - Make PresShell::GetVisualViewportSize aware of the dynamic toolbar height.

Previously, GetVisualViewportSize() returned the raw mVisualViewportSize, so
callers via ScrollContainerFrame::GetVisualViewportSize() were unaware of the
dynamic toolbar state. The toolbar awareness logic that was inline in
VisualViewport.cpp is moved into GetVisualViewportSize() itself, so all callers
get the correct size when the toolbar is collapsing or collapsed.

Original Revision: https://phabricator.services.mozilla.com/D297841

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 2036292 - Skip calling ScrollToVisual if the target frame is already in the visual viewport.

When scrollIntoView() is called on a position:fixed element that is already
visible in the visual viewport, ScrollFrameIntoVisualViewport() was still
calling ScrollToVisual() unnecessarily, causing a spurious visual scroll.

Add a check to skip the visual scroll if the element's rect (converted to
layout coordinates) is already contained within the current visual viewport.
Using layout coordinates is necessary since the visual viewport may have been
panned from the origin, in which case a viewport-origin check would give a
false negative for elements positioned beyond the visual viewport height.

Original Revision: https://phabricator.services.mozilla.com/D297842

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Users are suffering from annoying/unexpected content scrolling up and down while typing comments on facebook
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: yes
• Steps to reproduce for manual QE testing: It would be nice that someone (other than I) checks the facebook issue (bug 2028072) has been resolved.
• Risk associated with taking this patch: low
• Explanation of risk level: The change itself is relatively simple, also note that the change is only affecting on Android, not on desktops.
• String changes made/needed?: none
• Is Android affected?: yes

Comment 9

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/b6d9bf801927
https://hg.mozilla.org/releases/mozilla-beta/rev/c30955ee38a4
https://github.com/mozilla-firefox/firefox/commit/c2aee2c54c21
https://hg.mozilla.org/releases/mozilla-beta/rev/dfd325a96686

Comment 10

[Brindusa Tot, DTE]

The facebook issue (bug 2028072) seems to be an Android bug. Should this be also verified on Desktop version(on Windows, macOS or Ubuntu pperating system)?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

No.

Comment 12

[Mira Lobontiu (Android QA)]

I am still seeing the glitches when commenting on a post.
Testing was performed on the Firefox for Android Nightly 153.0a1 from 6/11 using a Huawei Pura 70 (Android 12).

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Mira, as far as I've tested I can no longer see bug 2028072 at all.

It would be nice to open a new bug for the glitches you saw. Thanks!

Comment 14

[Mira Lobontiu (Android QA)]

Filed Bug 2051595.
Thank you!