1927677 - [toolbar redesign] Navbar divider not hidden when navbar is hidden for webpage interactions

Status: RESOLVED FIXED

(Fenix :: Toolbar, defect, P2)

Description

[Petru-Mugurel Lingurar [:petru]]

Attachment: image.png

Steps to reproduce

1. Ensure the toolbar redesign is enabled in settings
2. Ensure the toolbar position is set at top
3. Access any website with input fields
4. Focus on an input field

Expected behavior

The keyboard appears with nothing on top - the navbar is hidden

Actual behavior

The keyboard appears but shows a divider on top - only the navbar is hidden, not it's divider also

Any additional information?

This is an old bug from the initial implementation of the divider which is separate from the navbar.
As a proposed implementation (mimicking what we do for the addressbar) the divider should be part of the navbar layout.

=========
Another related bug I saw while working on this:

1. Ensure the toolbar position is set at top
2. Get and close a microsurvey

Expected behavior

The top addressbar has no change

Actual behavior

The top addressbar gets a top divider

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Attachment: DividerForTopToolbar.mp4

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1927677 - part 1 - Remove divider from on top of the OS navigation bar r=#android-reviewers

initializeMicrosurveyPrompt is called in scenarios when we won't also show the navbar.
We wanted to show dividers on top of the bottom toolbar.
In this case the divider would be shown below it with no UX reason so it can be avoided.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1927677 - part 2 - Don't add top toolbar divider after microsurvey ends r=#android-reviewers

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1927677 - part 3 - Show the navbar divider in the same layout as the navbar r=#android-reviewers

Treating the navbar and it's divider as a whole will allow to apply changes like
visibility to both of them at the same time.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d64beaf4b61
part 1 - Remove divider from on top of the OS navigation bar r=android-reviewers,twhite
https://hg.mozilla.org/integration/autoland/rev/363f7a1b2f2b
part 2 - Don't add top toolbar divider after microsurvey ends r=android-reviewers,Roger
https://hg.mozilla.org/integration/autoland/rev/f49863193c13
part 3 - Show the navbar divider in the same layout as the navbar r=android-reviewers,Roger

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5d64beaf4b61
https://hg.mozilla.org/mozilla-central/rev/363f7a1b2f2b
https://hg.mozilla.org/mozilla-central/rev/f49863193c13

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Revert "Bug 1927677 - part 3 - Show the navbar divider in the same layout as the navbar r=#android-reviewers

This reverts commit 3acb5215f8a2e9addf690b8c04471273fd49cd21

Comment 8

[Phabricator Automation]

Comment on attachment 9437707 [details]
Revert "Bug 1927677 - part 3 - Show the navbar divider in the same layout as the navbar r=#android-reviewers

Revision D228989 was moved to bug 1930651. Setting attachment 9437707 [details] to obsolete.

Comment 9

[Chris Peterson [:cpeterson]]

Reopening this bug because the fix was backed out. Moving from beta blockers to release blockers.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1927677 - Show the divider in the same composable as the navbar r=#android-reviewers

This will allow for the internal functionality of the navbar to be hidden
when the keyboard is opened to also hide the divider.
Adding the divider in the same composable meant also adding the 1dp height
of the divider in the navbar composable.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef5de53ea5c0
Show the divider in the same composable as the navbar r=android-reviewers,harrisono

Comment 12

[Sandor Molnar[:smolnar]]

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

Comment 13

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

The patch landed in nightly and beta is affected.
:petru, 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-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Delia Pop (Android QA)]

Attachment: cca3e30b-4e25-4e94-8329-f9bbe0d33064.JPEG

Verified as fixed in Nightly 135.0a1 from 12/13 with Google Pixel 8 Pro (Android 14).

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 9441496 [details]
Bug 1927677 - Show the divider in the same composable as the navbar r=#android-reviewers

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Small bug fix planned to provide a polished UX of the redesigned toolbar when released.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Ensure the toolbar redesign is enabled in settings
  Ensure the toolbar position is set at top
  Access any website with input fields
  Focus on an input field
  Verify that there is no divider shown between web content and the keyboard.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small targetted patch verified by QA in Nightly.
• String changes made/needed:
• Is Android affected?: Yes

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9441496 [details]
Bug 1927677 - Show the divider in the same composable as the navbar r=#android-reviewers

The patch doesn't graft cleanly to beta and we are past our last beta. This is polish, we can reconsider a rebased patch for the planned dot release (request uplift of a rebased patch to mozilla-release once we have shipped 134.0). Thanks

Comment 17

[Petru-Mugurel Lingurar [:petru]]

Thank you!
Was a bit worried about potential conflicts with a few other toolbar redesign related patches being uplifted recently.
It's indeed a small detail, will check in the team (adding myself a NI for that) if we need this uplifted to release or can leave it to ride the trains.

Comment 18

[Petru-Mugurel Lingurar [:petru]]

Checked in the team, no need to uplift to 134.