1976535 - [composable toolbar] Using the navbar breaks searching mode

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, defect, P2)

Description

[Petru-Mugurel Lingurar [:petru]]

Steps to reproduce

1. Enable both the composable toolbar and the toolbar redesign feature from secret settings
2. Enable the expanded toolbar layout from the "Customize" settings screen
3. Tap on the toolbar from the home screen

Expected behavior

The toolbar enters in search mode

Actual behavior

Nothing happens

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Looking more into this it seems to be happening because HomeNavigationBar is instantiated before HomeToolbarComposable and it will create a new BrowserToolbarStore without the search related middlewares.

Ideally we should only use one BrowserToolbarStore per screen/fragment.
Since HomeNavigationBar only needs a simple store and HomeToolbarComposable needs more configuration it seems like we need to have a way to have HomeToolbarComposable instantiate first the BrowserToolbarStore and then pass it to HomeNavigationBar.

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Setting S4 since this bug only reproduces in the context of 2 not released features.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Considered adding the navbar in the HomeToolbarComposable / BrowserToolbarComposable. This could work but the contract of these classes is to return the toolbar layout so then these would do more than expected.
Also considered just passing to HomeNavigationBar / BrowserNavigationBar the BrowserToolbarStore instance from HomeToolbarComposable / BrowserToolbarComposable in a similar way to how it happens for the awesomebar but the navbar might be instantiated separately from the toolbar when the toolbar is to be shown at the top of the screen.

The easiest though is just to ensure the the navbar is instantiated after the toolbar composable. This would ensure that it will be able to access the same BrowserToolbarStore previously setup by the toolbar composable.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1976535 - Instantiate the navbar after the composable toolbar r=#android-reviewers

This would ensure the navbar will see the BrowserToolbarStore previously set up
by the composable toolbar.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1976535 - Use the same BrowserToolbarStore builder for both the navbar and the addressbar r=#android-reviewers

Previously both the navbar and the addressbar would build their own BrowserToolbarStore
but have this persisted as a ViewModel in the containing fragment.
This meant that whoever would try to build the store the second time would get the
initially built store, with potentially different middlewares set.

This patch brings one clear API to build the BrowserToolbarStore that can be used by multiple
separate features which would get the store instance properly configured to support them all.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8127efa0b48c
https://hg.mozilla.org/integration/autoland/rev/ad7a36306f2b
Use the same BrowserToolbarStore builder for both the navbar and the addressbar r=android-reviewers,Roger

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1976535 - Only instantiate the home navbar if the composable toolbar is enabled r=#android-reviewers

This brings a matching initialization flow to what is used in the browser screen.

Comment 8

[Cosmin Sabou [:CosminS]]

Backed out for fenix failures.

Push with failures

Failure log

Backout link

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bc69e8fbdfeb
https://hg.mozilla.org/integration/autoland/rev/60e5e776efad
Use the same BrowserToolbarStore builder for both the navbar and the addressbar r=android-reviewers,Roger
https://github.com/mozilla-firefox/firefox/commit/5ea4161e2d8a
https://hg.mozilla.org/integration/autoland/rev/a4c712ffead5
Only instantiate the home navbar if the composable toolbar is enabled r=android-reviewers,Roger

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/60e5e776efad
https://hg.mozilla.org/mozilla-central/rev/a4c712ffead5

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: WIP: Revert Bug 1976535 - Only instantiate the home navbar if the composable toolbar is enabled

This reverts commit 5ea4161e to get back in the state where we observed memory leaks