1906657 - Remove the isDisplayedWithBrowserToolbar hack from FenixSnackbar

Status: VERIFIED FIXED

(Fenix :: Toolbar, task, P1)

Description

[Petru-Mugurel Lingurar [:petru]]

As a followup to bug 1892762 which described a scenario in which snackbars might overlap the navbar we should look into ensuring that snackbars are only shown by the parent of the navbar - so that they can be shown above the navbar which can have double the height of the previous toolbar.

Fenix started adding a padding to the snackbar a long time ago - https://github.com/mozilla-mobile/fenix/issues/6413 to fake proper placement.
This padding was softly deprecated by the new FenixSnackbarBehavior (added in bug 1812518) which allowed to position the snackbar on top any other sibling, not just the toolbar.
But this is not currently used all throughout the app and there are still cases where the hardcoded padding will lead to UI bugs when the navbar is showing.
Fiddling with the FenixSnackbar code to consider whether the app shows a toolbar or a navbar, whether it's shown at the top or bottom is just technical debt in my opinion.

My proposal is to remove the isDisplayedWithBrowserToolbar param of FenixSnackbar.make(..) in favor of ensuring that snackbars that should be shown on top of the toolbar are only shown from the screen also showing the toolbar. And ensuring that we use a behavior to handle the proper placement without caring about actual height of the anchor.
Eg: this snackbar will overlap the navbar even if isDisplayedWithBrowserToolbar == true. This is easily solved by ensuring that the controller can ask the View (here the BrowserFragment) to show the snackbar (process in which we also improve the separation of responsabilities between view/controller - a controller should not directly touch the view).

Comment 1

[Joe M [:jmahon]]

Discussion notes:

• could use method that Petru used previously for deleting a bookmark so this hack would be unnecessary (use Snackbar binding that can observe AppStore for when state says we should show the snackbar, and show it in the relevant fragment instead of the Activity)
• current snackbar behavior could potentially cause unexpected regressions

(question: could this refactor wind up causing unexpected regressions?) -- with this change, snackbar would be shown on a specific screen, but would not necessary "follow" the user if they change to a different screen. This might actually be preferred, but it depends on the scenario. May be worth testing for scenarios when users are navigating quickly through History fragment.

Comment 2

[Joe M [:jmahon]]

planning poker notes:

• snackbars are difficult to test, they're on a lot of screens, so there's a chance that it might result in QA noticing something unusual
• discussion with UX might also take a bit of time (but we can minimize that conversation by proposing that we only mark the snackbar as "shown" if the full snackbar duration has passed. So, if the user only sees it for a second on the first screen, it will be re-shown on the second screen they look at.)

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Note: All snackbars shown on homescreen would currently obscure the bottom toolbar if it's scrollable because of this check

(view is ContentFrameLayout || !dynamicToolbarEnabled)

as we aren't using a ContentFrameLayout anywhere.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Note: From all current usages [1][2] it seems like in only one scenario the snackbar will be kept showing when the user navigates to other screens [3].

[1] https://searchfox.org/mozilla-central/search?q=FenixSnackbar.make%28&path=&case=false&regexp=false
[2] https://searchfox.org/mozilla-central/search?q=snackbarDelegate.show%28&path=&case=false&regexp=false
[3] https://searchfox.org/mozilla-central/rev/01aaa47e62a2015e7641f26ab0bc2bb00ab579b8/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/share/ShareFragment.kt#89-92

Comment 5

[Chris Peterson [:cpeterson]]

Increasing priority to P1 now that we're fixing toolbar phase 1's beta blockers.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 1 - Ensure home screen snackbars are shown on top of the toolbar r=#android-reviewers

Just needed to ensure that we have a handle for the snackbars.
Placing dynamicSnackbarContainer above toolbar_navbar_container was already
done in bug 1892762.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 2 - Cleanup all simple cases of adding a padding for the snackbar r=#android-reviewers

The browser screen and home screen are the only ones who show a toolbar and both
are set to properly place the snackbars above (Y axis) the toolbar already.

This patch cleans up scenarios where snacbkar paddings were added unneededly since
the snackbars were contained to other screens on which a toolbar is not shown.

Other more complex scenarios are left for later patches.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 3 - Use AppStore for handling browser/home menu actions r=#android-reviewers

Also ensure the home screen can observe and display snackbars from AppState,
something the browser screen is already doing.

This also fixes bug 1908678.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 4 - Show a snackbar on Android 13+ when users copy the current URL r=#android-reviewers

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 5 - Show snackbars for actions in the share dialog r=#android-reviewers

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 6 - Show error snackbars in the browser screen r=#android-reviewers

Comment 12

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 7 - Show snackbars after the user signs in r=#android-reviewers

This also fixes bug 1909167.

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1906657 - part 8 - Support the old addressbar also for the updated snackbars flow r=#android-reviewers

Saw in UI tests a scenario in which FenixSnackbarBehavior#layoutDependendsOn could be
called in an infinite loop for null dependency ids which would starve the Main thread
and lead to tests failing.
Using logs for debugging showed this not being an issue in real life.
But we should ensure that tests are not affected.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9e110f55633
part 1 - Ensure home screen snackbars are shown on top of the toolbar r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/69cf651f1376
part 2 - Cleanup all simple cases of adding a padding for the snackbar r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/c185c762153a
part 3 - Use AppStore for handling browser/home menu actions r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/f2e8fa7d8f06
part 4 - Show a snackbar on Android 13+ when users copy the current URL r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/b3104b4acdef
part 5 - Show snackbars for actions in the share dialog r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/b8756be64f4d
part 6 - Show error snackbars in the browser screen r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/1645c4156387
part 7 - Show snackbars after the user signs in r=android-reviewers,gl
https://hg.mozilla.org/integration/autoland/rev/f6f00b873b81
part 8 - Support the old addressbar also for the updated snackbars flow r=android-reviewers,gl

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b9e110f55633
https://hg.mozilla.org/mozilla-central/rev/69cf651f1376
https://hg.mozilla.org/mozilla-central/rev/c185c762153a
https://hg.mozilla.org/mozilla-central/rev/f2e8fa7d8f06
https://hg.mozilla.org/mozilla-central/rev/b3104b4acdef
https://hg.mozilla.org/mozilla-central/rev/b8756be64f4d
https://hg.mozilla.org/mozilla-central/rev/1645c4156387
https://hg.mozilla.org/mozilla-central/rev/f6f00b873b81

Comment 16

[Petru-Mugurel Lingurar [:petru]]

@ QA Can you please help verify this and the related snackbar issues?

Comment 17

[Delia Pop]

Verified as fixed in the latest Nightly 130.0a1 from 07/31 with Google Pixel 8 Pro (Android 14) and Xiaomi 12 Pro (Android 13).

• All the snackbars are displayed above the navigation bar/address bar, regardless of the address bar position.
• On devices with Android 13+, the "URL copied to clipboard" snackbar is no longer displayed, since the OS pop-up is triggered when copying the URL. Other instances where a snackbar is displayed under the OS pop-up will be added to 1802247 for further investigation.