1900622 - Fenix should react to Gecko's requests to hide the dynamic toolbar, to allow revealing text fields that are covered by the dynamic toolbar

Status: ASSIGNED

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

Description

[Alex Jakobi [:ajakobi]]

After closing bug 1855990, follow up by implementing means to hide the dynamic toolbar on Fenix.
Involve QA for testing this.

Comment 1

[Alex Jakobi [:ajakobi]]

Attachment: Bug 1900622 - Add capabilities to hide dynamic toolbar on Fenix. r=botond,#android-reviewers

Comment 2

[Alex Jakobi [:ajakobi]]

Attachment: 1900622-testpage.html

Comment 3

[Alex Jakobi [:ajakobi]]

Steps to test for QA:

1. Load attached testpage with Android device with pref. dynamic toolbar enabled.
2. Tap into the input field at the bottom of the page, which will cause the keyboard to be shown and scroll the now focused input field into view

Expected result:

• The dynamic toolbar gets hidden such that the input field is fully visible
• No other changes to the dynamic toolbar behavior

Comment 4

[Delia Pop]

Attachment: Untitled.png

Hi, Alex,
I tried to reproduce this issue with Samsung Galaxy A53 5G (Android 14) and Google Pixel 8 Pro (Android 14) in latest Nightly 129.0a1 from 07/03 and GVE, but the input field is not displayed when loading the attached testpage in Fenix.

I was however able to reproduce this using the test pages provided in this comment, moreover with Input field too low. This is reproducible with all latest releases (Nightly 129.0a1, Beta 129.0b9, RC 129.0 build 1).

Is there something I'm missing in order to reproduce this issue with the attached testpage? Thank you!

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: The correct one

Alex somehow attached an HTML showing the source HTML. This one should work.

Comment 6

[Alex Jakobi [:ajakobi]]

Thanks Hiro!
Delia, my apologies, it's my first time adding QA flags to a bug. The change for this bug is not yet landed, I'll let you know as soon as it has landed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/331cc5c5a9d9
Add capabilities to hide dynamic toolbar on Fenix. r=android-reviewers,tthibaud

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0954f6f12226
Fix ktlint failure. CLOSED TREE

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/331cc5c5a9d9
https://hg.mozilla.org/mozilla-central/rev/0954f6f12226

Comment 10

[Lina Butler [:lina]]

Hi Alex! It looks like the new expand collapse expand test is a high-frequency intermittent (bug 1908553). I've seen it pop up in a couple of my Try runs, and quite a few failures on Autoland. Could you have a look, please? ☺️ Thanks!

Comment 11

[Norisz Fay [:noriszfay]]

Backed out for causing Bug 1908553 and ktlint failure

Backout link

ktlint - Push with failures

ktlint - Failure log

Comment 12

[Alex Jakobi [:ajakobi]]

Lina, Norisz, thanks for reporting an backing this out. I will take a look.

Comment 13

[miralobontiu]

Hi Alex,
I'm removing the qe-verify+ label for now, as the change is not landed yet.
Please add it again when QA can test the change.
Thank you

Comment 14

[Alex Jakobi [:ajakobi]]

Understood, thanks for taking care of this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e422af6bf16
Add capabilities to hide dynamic toolbar on Fenix. r=android-reviewers,tthibaud

Comment 16

[Sandor Molnar[:smolnar]]

Backed out for causing ktlint failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/711b6c719a341215095a97b4a5ca7e10facfdbb0

Push with failures

Failure log -> 10:57:47.185 [main] INFO com.pinterest.ktlint.cli.internal.ReporterAggregator - "json" report written to /builds/worker/checkouts/gecko/mobile/android/android-components/build/reports/ktlint/ktlint.json

Failure log ->TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/android-components/components/feature/toolbar/src/test/java/mozilla/components/feature/toolbar/ToolbarBehaviorControllerTest.kt:24:1 | Unused import (standard:no-unused-imports)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e90d4466e409
Add capabilities to hide dynamic toolbar on Fenix. r=android-reviewers,tthibaud

Comment 18

[Alex Jakobi [:ajakobi]]

Linting error should be fixed now.

Comment 19

[Serban Stanca [:SerbanS]]

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

Comment 20

[Alex Jakobi [:ajakobi]]

Adding qe-verify+ label again. Please let me know should you need more information or face any issues.

Comment 21

[miralobontiu]

Attachment: az_recorder_20240823_151035.mp4

Verified on the latest Fenix Nightly 131.0a1 build from 8/23 with a Google Pixel 6 (Android 15), and a Samsung Galaxy S24 (Android 14).
As it can be seen in the short video attached:

• on Nightly when the field is tapped, the toolbar is dynamic,
• on RC when the field is tapped, the toolbar is displayed above it.

Tested on this test page: https://bugzilla.mozilla.org/attachment.cgi?id=9412359

Alex, should any pref be modified in the about:config page, or is the scenario above suffice?

Comment 22

[Alex Jakobi [:ajakobi]]

Hi Mira, thanks for checking! The change is not behind any pref. I just checked on Fenix Nightly 131.0a1 with a Google Pixel 8 (Android 14) and see the same result as shown in the video you've attached.

The dynamic toolbar gets hidden when the input field is focused, that's good. However, when the dynamic toolbar comes back into view, its content is not shown, only a very thin line is seen where the top of the dynamic toolbar should be. That is definitely not intended.

I'll check.

Comment 23

[Alex Jakobi [:ajakobi]]

Attachment: Backed out changeset e90d4466e409 (Bug 1900622)

Back out due to regression found by QA in comment #21.

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bef0c84e402b
Backed out changeset e90d4466e409 r=botond,android-reviewers,tthibaud

Comment 25

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 26

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/bef0c84e402b

Comment 27

[Botond Ballo [:botond]]

Reopening since the fix was backed out.

Comment 28

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1900622 - Avoid individually scrolling addressbar if shown together with the navbar r=#android-reviewers

If the dynamic toolbar feature is enabled then these two will be scrolled together.
If the addressbar is permitted to also be scrolled independently then it would be scrolled
two times.

Comment 29

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1900622 - Avoid individually scrolling addressbar if shown together with the navbar r=#android-reviewers

If the dynamic toolbar feature is enabled then these two will be scrolled together.
If the addressbar is permitted to also be scrolled independently then it would be scrolled
two times.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/351de2a3a578
Avoid individually scrolling addressbar if shown together with the navbar r=android-reviewers,harrisono

Comment 31

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/351de2a3a578

Comment 32

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 33

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

The patch landed in nightly and beta is affected.
:ajakobi, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval. Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox131 to wontfix.

For more information, please visit BugBot documentation.

Comment 34

[Botond Ballo [:botond]]

(Reopening since the patch that landed is just part of the fix.)

Comment 35

[Alex Jakobi [:ajakobi]]

Working on getting the necessary change re-landed.

Comment 36

[Alex Jakobi [:ajakobi]]

Due to updates to interactive-widget, hiding the toolbar in resizes-visual does not work and is not the right approach to begin with. See discussion in bug 1920019 for details.

Apart from that, refactoring the dynamic toolbar behavior is an initiative in the making, this will likely implement a different approach to hiding the toolbar and resolve this bug.

Until then, we will pause work on this bug.

Changes in D213962 are complete.

Comment 37

[Petru-Mugurel Lingurar [:petru]]

@Alex @Hiro
Asked Channing on whether we still need to have this block the navbar release.
Curious about your opinion also and if you know how big of an issue this is in real world - are you aware of any website that is affected?

Comment 38

[Alex Jakobi [:ajakobi]]

I personally don't think this should be a blocker to the Navbar release. I don't know of any production websites that trigger this bug. It's rather an edge case with pages being vertically larger than 100svh but smaller than 100lvh and those must have 'interesting' elements (e.g. input field) close enough to the bottom for the toolbar to overlap it when you want to interact with it. You can work around it by scrolling so that the dynamic toolbar hides. Additionally, this bug is part of production code for an unknown time already and we're not seeing outside reports of this as far as I'm aware.

Comment 39

[Petru-Mugurel Lingurar [:petru]]

Thank you for the updates
Based on Channing's input also

I'm fine with not making this a release blocker but we should keep an eye to see if we get any user complaints.

I will remove this from the list of navbar blockers.