1937785 - Viewport resizes with `overlays-content` result in rendering issues on Android 10

Status: RESOLVED FIXED

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

Description

[Petru-Mugurel Lingurar [:petru]]

Attachment: overlaysContentIssue.mp4

Steps to reproduce:

• Oppo A15s device with Android 10
• access https://viewport-resize-behavior.netlify.app/interactive-widget/overlays-content/
• Tap on the input element

Expected result:

• No update for the viewport or what is being rendered

Actual result:

• The rendered content is smaller than the available viewport
• The scrollbars do extend to the full viewport

✱ This assumes dom.interactive_widget_default_resizes_visual preference is enabled
✱ No issues observed when using other viewport resize modes.

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Attachment: WithoutViewportResizes.mp4

Another note about the issue being even worse with dom.interactive_widget_default_resizes_visual disabled: it doesn't even need the keyboard to be showing.

Comment 2

[Petru-Mugurel Lingurar [:petru]]

And another note that the issue might be device specific.
I was looking into this because of some user reported issues on Fenix - bug 1933488 which seemed related to this functionality.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I haven't looked yet, but if it's specific to Android 10, it's probably related to bug 1920755, which means the way we use to deduce the keyboard height is somewhat broken.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

This isn't specific to Android 10, I can see the blank area on Android emulator 7.0.

On chrome with overlays-content mode the dynamic toolbar doesn't move. That kinda makes sense since moving the dynamic toolbar basically changes the visual viewport.

Though I haven't yet found out where the underlying problem is, we need to tweak while moving the toolbar.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I wondered why I didn't realize this issue when I was working on bug 1831649. That's because this is a regression.

The regression range I could get is;

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20aa234dca65c33996123f496a8ad82ea5e87ba5&tochange=6e6d0fda9ab44e3931284a06e04241fc94ef844c

Bug 1918178 looks very likely the casue.

Comment 6

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

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Use `min` to chomp the given string. r?botond.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Fixed a typo in a comment in APZCTreeManager::ComputeFixedMarginsOffset. r?botond

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Exclude the dynamic toolbar from the composition bounds in the case of `overlays-content` with software keyboard. r?botond

Comment 10

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

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

For more information, please visit BugBot documentation.

Comment 11

[Botond Ballo [:botond]]

We are tracking this in FFXP-3198. I'm marking this as an S3 for now since overlays-content is a non-default interactive-widget mode that websites need to opt into and we're not yet aware of production websites impacted by this bug. (If anyone does know such websites, please mention them in a comment.)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Make PresShell::GetMobileViewportManager return a raw pointer. r?botond

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Remove nsPresContext::mKeyboardHeight and introduce MobileViewportManager::mPendingKeyboardHeight. r?botond

There's a race condition where the keyboard height change notification
has arrived but nsDocumentViewer size change hasn't yet arrived.

MobileViewportManager::mPendingKeyboardHeight is introduced to fix the race.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Though I don't yet have any idea how the bad rendering happens, I've narrowed down and reached to a point where the undesirable composition bounds from UpdateCompositionBoundsForRCDRSF is used.

On the content side, the call site is this ComputeScrollMetadata in WebRenderLayerScrollData::Initialize, it's propagated to APZ.

Then on the APZ side, the problematic use site of the composition bounds is this RecalculateLayoutViewportOffset in AsyncPanZoomController::SetVisualScrollOffset. And it calls KeepLayoutViewportEnclosingVisualViewport.

The layout viewport mLayoutViewport value is NOT including the dynamic toolbar, whereas the visual viewport IS including the dynamic toolbar because it calculated based on the composition bounds.

So, this inconsistency is the culprit. The layout viewport is correct, while the keyboard is shown on overlays-content mode it doesn't need to care about the dynamic toolbar.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e61d106beb8
Use `min` to chomp the given string. r=botond,geckoview-reviewers,owlish.
https://hg.mozilla.org/integration/autoland/rev/cb38950d4787
Fixed a typo in a comment in APZCTreeManager::ComputeFixedMarginsOffset. r=botond
https://hg.mozilla.org/integration/autoland/rev/7f3f99057dbe
Make PresShell::GetMobileViewportManager return a raw pointer. r=botond
https://hg.mozilla.org/integration/autoland/rev/0e95190b1c76
Remove nsPresContext::mKeyboardHeight and introduce MobileViewportManager::mPendingKeyboardHeight. r=botond
https://hg.mozilla.org/integration/autoland/rev/9ea29b89cbf5
Exclude the dynamic toolbar from the composition bounds in the case of `overlays-content` with software keyboard. r=botond,geckoview-reviewers,owlish
https://hg.mozilla.org/integration/autoland/rev/ca4ff44a38f3
apply code formatting via Lando

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/009de84d1d82
Backed out 6 changesets for causing Android reftests failures in retained-dl-async-scrolled-1.html. CLOSED TREE

Comment 17

[Serban Stanca [:SerbanS]]

Backed out for causing Android reftests failures in retained-dl-async-scrolled-1.html.

• Backout link
• Push with failures
• Failure Log
• Failure line: REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/display-list/retained-dl-async-scrolled-1.html == layout/reftests/display-list/retained-dl-async-scrolled-1-ref.html | image comparison, max difference: 54, number of differing pixels: 3000

Comment 18

[Hiroyuki Ikezoe (:hiro)]

I haven't been able to reproduce the failures locally nor on try serves. It's very likely that the failures are caused by bug 1918577, i.e. the software keyboard keeps opening there once a reftest has opened it.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1937785 - Set inputmode="none" in some reftests to avoid opening the softwarekeyboard on Android. r?masayuki

The opened keyboard keeps there, and it makes subsequent tests fail
unfortunately.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

I am not 100% sure that D235092 fixes the all failure, it's quite hard to tell it on try runs.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec7b41129a2e
Use `min` to chomp the given string. r=botond,geckoview-reviewers,owlish.
https://hg.mozilla.org/integration/autoland/rev/cda233815bc4
Fixed a typo in a comment in APZCTreeManager::ComputeFixedMarginsOffset. r=botond
https://hg.mozilla.org/integration/autoland/rev/f7e8b3a12560
Make PresShell::GetMobileViewportManager return a raw pointer. r=botond
https://hg.mozilla.org/integration/autoland/rev/a468ba3d2722
Remove nsPresContext::mKeyboardHeight and introduce MobileViewportManager::mPendingKeyboardHeight. r=botond
https://hg.mozilla.org/integration/autoland/rev/d47c3cbb83ff
Exclude the dynamic toolbar from the composition bounds in the case of `overlays-content` with software keyboard. r=botond,geckoview-reviewers,owlish
https://hg.mozilla.org/integration/autoland/rev/b822c75c402f
Set inputmode="none" in some reftests to avoid opening the softwarekeyboard on Android. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/909971314e65
apply code formatting via Lando

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ec7b41129a2e
https://hg.mozilla.org/mozilla-central/rev/cda233815bc4
https://hg.mozilla.org/mozilla-central/rev/f7e8b3a12560
https://hg.mozilla.org/mozilla-central/rev/a468ba3d2722
https://hg.mozilla.org/mozilla-central/rev/d47c3cbb83ff
https://hg.mozilla.org/mozilla-central/rev/b822c75c402f
https://hg.mozilla.org/mozilla-central/rev/909971314e65

Comment 23

[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-firefox135 to wontfix.

For more information, please visit BugBot documentation.