1812821 - [Bug]: Main Menu: Stop/Reload icon is not updated when the menu is open

Status: RESOLVED DUPLICATE of bug 1964773

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

Description

[Jonathan Almeida [:jonalmeida]]

From github: https://github.com/mozilla-mobile/fenix/issues/25166.

Steps to reproduce

1. Start loading a page (that takes some time to load)
2. Open the Main Menu: it shows the Stop button ("X")
3. Wait for the page to load (progress bar disappears)
4. Check the Stop/Reload button

Expected behaviour

Stop/Reload button now acts as Reload, and so its icon should change to Reload.

Actual behaviour

Stop/Reload button now acts as Reload, but its icon still shows Stop ("X").
You need to close and reopen the Main Menu to see the updated icon for this button.

Device name

Samsung A52s

Android version

Android 12

Firefox release type

Firefox

Firefox version

100.1.1
103.2.0
104.2.0
105.0b6
106.0a1
106.1.0
108.0a1

Device logs

No response

Additional information

No response

┆Issue is synchronized with this Jira Story

Change performed by the Move to Bugzilla add-on.

Comment 1

[Piotr Henryk Dabrowski]

Bug still present in Firefox 113.2.0.

Comment 2

[Piotr Henryk Dabrowski]

Attachment: Screen_Recording_20220514-071255_Firefox.mp4

Comment 4

[Chris Peterson [:cpeterson]]

The stop/reload button will be removed from the menu in our new menu design (bug 1898393) and moved to the toolbar (bug 1894470).

Comment 5

[tchoh]

Investigation Findings: DefaultToolbarMenu Issue

After conducting an investigation, I’m putting this ticket on hold. Given the uncertainty surrounding the Menu redesign, addressing this issue may not be a worthwhile use of time at this stage. However, I’ve compiled a detailed report on my findings for future reference, should this need to be revisited:

---

Findings

Root Cause

The issue originates in DefaultToolbarMenu.kt.
I attempted several solutions, the two main ones being:

1. Pulling refresh into its own value within the class and passing that reference into the listOf for BrowserMenuItemToolbar.
2. Directly accessing the ImageView of the TwoStateButton. However, this approach caused a race condition between the static row and the sticky row.

Key Observations

A deeper dive into BrowserMenuItemToolbar revealed the main issue: multiple instances of BrowserMenuItemToolbar are being created.

• The first instance is created via BrowserMenuItemToolbar.
• The second instance is created from the same stack trace, but without a defined parent layout.
• The third instance is created via StickyItemLinearLayoutManager.

This behavior specifically impacts the use case where the toolbar is positioned at the top of the screen.

Additionally, every time the user scrolls back to the static row position, bind() is being called on the static row.

---

Problem Details

When the Menu is created, a sticky row is generated, which is rendered on top of the static row created by BrowserMenuItemToolbar. The issue varies depending on the toolbar position:

1. Address Bar at the Bottom

• The Stop/Refresh button updates correctly based on page load completion.
• The sticky row handles the reference and updates the UI properly.
• The static row also updates properly, but it is not visible to the user.

2. Address Bar at the Top

• Both the static row and the sticky row exist.
• The static row reference is updated correctly, but it is not visible to the user.
• The sticky row remains visible, but no reference exists to update its UI.

---

Conclusion

The primary issue lies in the redundant creation of BrowserMenuItemToolbar instances and the disconnect in managing UI references between the static and sticky rows. Resolving this would likely require addressing how rows are managed when the toolbar is created, particularly for the top-of-screen use case.

For now, I recommend deferring work on this ticket until the Menu redesign has a direction to go as this problem doesn't exist in the redesign.

Comment 6

[Zac McKenney [:zmckenney]]

The Menu Redesign will also need to incorporate navigation buttons to be compatible with the current toolbar but I'm not sure they'll be incorporated in the same way. Thanks for the detailed investigation writeup! We'll want to keep this in mind for future work as it seems the bigger issue is the persistence of the buttons and the updates on themselves. I agree that given the S4 severity and your writeup we can wait on this and evaluate with any new menu redesign.

Comment 7

[[:skhan]]

Hi, this will be solved our new menu redesign project. Specifically this task
https://bugzilla.mozilla.org/show_bug.cgi?id=1964773

Comment 8

[Jonathan Almeida [:jonalmeida]]

Thanks skhan, feel free to close similar kinds of issues you come across.