1909181 - Blank area at bottom of screen when zoomed in on Android with dynamic toolbar

Status: VERIFIED FIXED

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

Description

[Kestrel]

Attachment: Zoomed view showing blank area at bottom of screen

STR:

1. Have the dynamic toolbar enabled (default) on latest Android Nightly 130.0a1.
2. Visit https://en.wikipedia.org.
3. Zoom in.
4. Scroll down.

Blank area appears at bottom of screen, larger with increased zoom.

Does not happen with static toolbar.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=980f79f57946301607ffb6a3d091ca965f9c4140&tochange=111141cbf85939ddd0511a7456ec242487a0a60a

Regressed by Bug 1788504.

Comment 1

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

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

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hiro mind taking a look? You're more familiar with MVM stuff than I am :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The code regressed is this GetScrollPortRectAccountingForMaxDynamicToolbar call in ScrollContainerFrame::BuildDisplayList. Since bug 1788504, the rect became slightly smaller than before because the max dynamic toolbar height is divided by the current pinch zoom scale.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1909181 - Revert bug 1788504 partially. r?emilio

Since bug 1788504 the root scroll container's clipped region had been
over-clipped because GetScrollPortRectAccountingForMaxDynamicToolbar returns
the toolbar height divided by the current pinch zoom ratio. For example, given
that the toolbar height is 60px (in screen units) and the pinch zoom ratio is
2.0, then GetScrollPortRectAccountingForMaxDynamicToolbar returns 30px (in CSS
units), it's not sufficient for the clipped region.

The JUnit test in this change fails without this change, succeeds with this
change.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case I used to tell the IntersectionObserver in Chrome divides the toolbar height by the current pinch zoom scale or not

I also wondered that Chrome's IntersectionObserver divides the toolbar height by the current pinch zoom scale or not, but it's quite hard to tell since IntersectionObserver doesn't check the intersections between the visual viewport, it rather checks the layout viewport, thus while the document is zoomed in, the intersections happen outside of the visual viewport.

So I will punt off the question here. I think some edge cases will reveal the question. (My intuitive answer is Chrome uses the minimum scale, I am quite unsure though)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1909181 - Make viewport units independent from pinch-zoom scale. r?emilio

Since bug 1788504, with dynamic toolbar, viewport units had depended on
pinch-zoom scale. That's totally wrong. Viewport units should be independent
from pinch-zoom scale. A couple of examples have been attached in bug 1910518.

Comment 7

[Pascal Chevrel:pascalc]

This is a 130 regression, can we have the fix landed before we merge to beta? Thanks

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I'd want to, but it depends on #geckoview-reviewers.

Comment 9

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

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

Comment 10

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

The severity field is not set for this bug.
:hiro, could you have a look please?

For more information, please visit BugBot documentation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d92be4e0f88
Revert bug 1788504 partially. r=emilio,geckoview-reviewers,ohall
https://hg.mozilla.org/integration/autoland/rev/f56f6d107cfe
Make viewport units independent from pinch-zoom scale. r=emilio

Comment 12

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/9d1048ad548cb2b1b83086814b2b11c0f3bb37fe

Push with failures

Failure log #1

Failure log #2

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: mozilla-test-fail-screenshot_52q3__8l.png

With renaming the test file name from test_bug1909181.html to test_viewport_units_with_dynamic_toolbar.html. There was no suspicious failure; https://treeherder.mozilla.org/jobs?repo=try&revision=1381e80713ab682d8a56e8030ebbc8c8d213598b

At least for the test failure on test_bug1909181.html, it's somehow failed by the previous test, test_bug1836801.html. Attaching file is a screenshot when test_bug1909181.html failed. It's fullscren state.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

CCing Timothy. ^

Given that there's no test failure with renaming the test file, I will land patches here with the renaming change (tomorrow).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb80cb85023b
Revert bug 1788504 partially. r=emilio,geckoview-reviewers,ohall
https://hg.mozilla.org/integration/autoland/rev/7ee9d19b1659
Make viewport units independent from pinch-zoom scale. r=emilio

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/eb80cb85023b
https://hg.mozilla.org/mozilla-central/rev/7ee9d19b1659

Comment 17

[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.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9416712 [details]
Bug 1909181 - Make viewport units independent from pinch-zoom scale. r?emilio

Beta/Release Uplift Approval Request

• User impact if declined: Users will see un-rendered area on pinch-zoomed in contents
• 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): It's basically harmless changes, it will never cause any critical issues such as crash, it does just expand the rendering area user is seeing
• String changes made/needed: none
• Is Android affected?: Yes

Comment 19

[Pascal Chevrel:pascalc]

Hiro, should eb80cb85023b also be uplifted to beta?

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Yeah, both changes in this bug need to land together. Thanks.

Comment 21

[Pascal Chevrel:pascalc]

Comment on attachment 9416712 [details]
Bug 1909181 - Make viewport units independent from pinch-zoom scale. r?emilio

Approved for 130 beta 5, thanks.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1bc9f6e1bc46
https://hg.mozilla.org/releases/mozilla-beta/rev/09d011bad5ac

Comment 24

[Adina Petridean QA [:apetridean]]

Verified as fixed on Nightly 130.0a1 from 16.08.2024 and Firefox 130.0b6 with Xiaomi Pad5 (Android 13) , Oppo Find N2 Flip (Android 14) and Oneplus 6T (Android 11).