1908249 - [toolbar redesign] Navbar still steals space from viewport when keyboard appears (even though navbar is hidden)

Status: VERIFIED FIXED

(Fenix :: Toolbar, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Steps to reproduce

1. Load attached testcase in Firefox Nightly on Android
2. Tap the textfield to bring up the keyboard, and see how the pink border changes.

Expected behavior

Eventually/ideally, the bottom edge of the pink border should not move; it should just be covered up by the keyboard (that's bug 1007286).
For now, though: the bottom edge of the pink border should be flush with the keyboard.

Actual behavior

The bottom edge of the pink border is shifted upwards from the keyboard. I think it's shifted upwards by an amount that would be required to dodge the navbar, if the navbar were showing. But the navbar is not showing; it hides when the board comes up.

Device information

• Firefox version: 130.0a1 2024-07-15
• Android device model: Pixel 8
• Android OS version: 14

Comment 1

[Daniel Holbert [:dholbert]]

This is causing noticeable breakage in-the-wild at sites that have a bottom toolbar, e.g. X/twitter. Screenshots coming up.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 with default settings (note the viewport background-color gets painted beyond the bottom border)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1 with "scroll to hide address bar and toolbar" turned off (note the awkward gray space between bottom border and keyboard)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug manifesting on X/Twitter in Settings | Search Settings UI - note akwward space below Twitter's bottom icon-row

Here's how this looks on a real-world site. STR for getting to this particular page are:

1. sign in to https://x.com/
2. Tap your avatar at top-left
3. Tap "settings and privacy"
4. Tap "search settings"

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

Expected behavior

Eventually/ideally, the bottom edge of the pink border should not move; it should just be covered up by the keyboard (that's bug 1007286).
For now, though: the bottom edge of the pink border should be flush with the keyboard.

For comparison:

• Chrome gives me the "Eventually/Ideally" expected-behavior here (they cover up the bottom edge of the pink border, per bug 1007286).
• Firefox Beta and Release (129 and 128) give me the "For now" expected-behavior -- the bottom edge of the pink border is flush with the keyboard.
• Firefox Nightly gives me "actual behavior" (bottom border floating some distance away from the keyboard), presumably due to the introduction of the navbar UI, which dynamically takes up extra space which we're not properly accounting for.

Comment 6

[Daniel Holbert [:dholbert]]

Mike, maybe you could take a look at some point - I see you've been poking various navbar-related bugs. Thanks!

Comment 7

[Petru-Mugurel Lingurar [:petru]]

The issue depends on how we use setDynamicToolbarMaxHeight.
We set the value of both the addressbar and navbar
But then when the keyboard is opened only the addressbar is shown.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1908249 - Get engine view in sync with navbar visibility r=#android-reviewers

The navbar has an internal functionality of getting hidden whenever the keyboard is shown.
This would leave GeckoView still configured to show dynamic page elements in the space left
by considering both the address bar and the navbar heights and so would improperly place these.

The proposed fix will have the navbar inform when it's visibility depending on the keyboard
state changes so that it's integrators can sync this with the engine view.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

This bug made me realize that the navbar design is quite debatable. As of now using setDynamicToolbarMaxHeight to fix this bug would kinda make sense, but once after we shipped interactive-widget (bug 1831649) (which is about to land), it would not make sense on interactive-widget=resizes-visual or interactive-widget=overlays-content since changing the dynamic toolbar max height will cause reflows.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Well, "reflows" was quite ambiguous. From the spec

overlays-content
    Interactive UI widgets MUST NOT resize the initial viewport nor the visual viewport.
resizes-visual
    Interactive UI widgets MUST resize the visual viewport but MUST NOT resize the initial viewport. 

Changing the dynamic toolbar max height changes the initial viewport.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: FixDynamicToolbarSizeInNormalTabs.mp4

@hiro Great that you saw this and thanks for the feedback.
What would be your recommendations then?
As seen in this attachment we really do cycle between showing two toolbars (the addressbar and the navbar) and showing just one (the addressbar).

Comment 12

[Hiroyuki Ikezoe (:hiro)]

There's no way other than adding a new API for the navibar.

The current two APIs;

• setDynamicToolbarMaxHeight changes the layout viewport (equals to the initial viewport in the spec in most cases)
• setVerticalClipping doesn't change the layout viewport, (it does change the visual viewport) and it also changes position:fixed elements positions

For the navibar we need a new API which doesn't change the layout viewport in the case of interactive-width:resizes-visual or interactive-widget-overlays-content, does change the layout viewport in the case of interactive-widget=resizes-content.

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Seeing that both this and bug 1831649 are ready to land
I want to align on how/which to land.
To me it seems like like for this ticket we fix an important bug, Youtube, Maps, Twitter and others being affected
While bug 1831649 is an improvement.
So we should definitely land this.
But then not sure how big the impact of the reflow mentioned above that this would cause is.
Should we land the patches for both tickets now? Should bug 1831649 wait for the other improvement noted above?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I just landed the change an hours ago, it failed due to a conflict though, I am going to land the change again.

Anyways, so for now it's okay to land them both, that's said we definitely need a new API for the navi bar behavior, please open a bug for that. Using setDynamicToolbarMaxHeight does not mean only it causes redundant reflows but also cause visual issues such as this bug.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d735fac675d9
Get engine view in sync with navbar visibility r=android-reviewers,skhan

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d735fac675d9

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Petru, did you file a bug for the new API? Mind CCing me in that bug? Thanks!

Comment 18

[Petru-Mugurel Lingurar [:petru]]

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

Petru, did you file a bug for the new API? Mind CCing me in that bug? Thanks!

Did that now, thanks for the reminder!

Comment 19

[Petru-Mugurel Lingurar [:petru]]

@ QA please help validate the fix.
This can easily be tested before / after on youtube or other websites that have their own bottom toolbar.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Thanks!

Comment 21

[Delia Pop]

Attachment: QA (2).mp4

Verified as fixed in the latest Nightly 131.0a1 from 08/08 with Xiaomi 12 Pro (Android 13) and Lenovo Yoga Tab 11 (Android 12).

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I have an alternative compromised solution here. Now our default behavior on showing the software keyboard is interactive-widget=resizes-visual, which means the original issue doesn't happen even if we don't explicitly call setDynamicToolbarMaxHeight with a different value.

So the compromised solution is backing out the patch here. The original issue will still happen if there's any site specifying interactive-widget=resizes-content explicitly, but I suppose there are no such site (yet), since the feature is pretty new and none of browser's default is the resizes-content.

I would say this issue would less severe with the new default resizes-visual mode, rather broken layout such as bug 1916154 would be more severe.

Daniel, Petru what do you think about this compromised solution?

Comment 23

[Daniel Holbert [:dholbert]]

I'm not familiar with the code in the patch that landed here, or the internal details of the interactive-widget stuff (other than having run up against it when diagnosing site-report bugs), or how likely sites are to request one or the other; so I'm happy to defer to your judgement. :)

The only possible-pitfall I'd note from the proposed backout would be: I think (?) it'd mean this bug would regress for users on release versions until we ship bug 1916002. (As you say: "this issue [is] less severe with the new default resizes-visual mode" -- that's only the default on Nightly for now, pending bug 1916002 to let it ride the trains to release.) i.e. whatever benefits we got from the patch that landed here would be lost for release users, until we're ready to ship resizes-visual to release.

(You're of course already aware of that & perhaps timing your proposed backout accordingly, but I wanted to mention that just in case.)

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Yeah, that's totally right. The default resizes-visual mode is gated by dom.interactive_widget_default_resizes_visual pref, so we can expose the pref value via GeckoRuntimeSettings so that the patch for this bug can be revised with referring the pref. Once after we shipped bug 1916002, we can drop the code entirely.

Comment 25

[Petru-Mugurel Lingurar [:petru]]

@Hiro So if we'd be okay to allow for the issue described here then bug 1916154 (which was presented as a potential blocker for the navbar feature) would be avoided?
Was curious about how these scenarios are handled on iOS and they are not.
So having parity with iOS 😅 while avoiding bug 1916154 sounds good to me.