Menu items are not properly displayed with tab overflow menu regroup flag

[frankjrp]

Bromite version

Version: 88.0.4324.207
Arch: arm
Android version: 5.1
Device model: Moto G1 - XT1033

Is this bug about the SystemWebView?

No

Is the bug reproducible with latest version?

Yes

Can the bug be reproduced with corresponding Chromium version?

No

Is the bug a crash?

Yes

Describe the bug

When opening the menu its header is empty, the view source icon is broken and the bookmark all tabs menu item is missing.

Steps to reproduce the bug

Steps to reproduce the bug:

1. Install the latest Bromite version (88.0.4324.207);
2. Open the menu;
3. See that the menu header is empty, the View source icon is broken and the bookmark all tabs menu item is missing.

Expected behavior

When opening the menu all the menu items are displayed properly, as seen in the previous Bromite version.

Question: What was wrong with the exit and view source menu items icons? Apparently they were working fine for me.

PS: I know this phone is very old, its my backup device, but everything was working fine in the previous version.

Screenshots

[SumatraPeter]

I'm using the same version via F-Droid on a tablet with even an older Android version i.e. 5.0.1, and I see no blank menu header (topmost item is New tab), plus the Bookmark all tabs menu item is visible (and works). The View source icon is the same as in your screenshot, but I don't remember what it looked like before, so cannot say for certain whether it is broken or not (as a document-type icon it looked fine to me).

[chromer030]

I'm using same version, no issue. but perhaps this occurs in locales other than English.

[frankjrp]

I found out what's going on.

If I disable the flag #tabbed-app-overflow-menu-regroup I can see all the menu items correctly. But in the old menu layout.

If I enable this flag, the menu is broken as described above.

About the View source icon: I think @csagan5 changed this menu item icon. But the other one was prettier.

@SumatraPeter and @chromer030 can you guys please check if that flag is enabled and you're using the new menu layout?

Old menu layout:

[chromer030]

I'm using old menu by default.

[csagan5]

@frankjrp can you provide steps to reproduce it, starting from a clean installation?

[SumatraPeter]

I'm using old menu by default.

Same here, flag enabled, disabled or default gives me the same old menu. New menu must be for newer Android versions only.

[frankjrp]

can you provide steps to reproduce it, starting from a clean installation?

@csagan5 Ok, I did a clean installation.

1. When opening Bromite menu after this clean installation, it shows the old layout with all the items correctly arranged. The flag #tabbed-app-overflow-menu-regroup is set to Default.

2. When setting this flag to Enabled - relaunch then close Bromite and open again - the menu shows the new layout, but the items are not properly displayed as described above.

It seems that it's working only when this flag is not enabled.

[hbarsaiyan]

I also have the same problem. The icons are not displaying for some items. I am on LineageOS 16.

[frankjrp]

@hbarsaiyan was it working on the previous version?

[hbarsaiyan]

@hbarsaiyan was it working on the previous version?

No the old menu was showing in default and when i enable the flag it shows like this.

[csagan5]

2. When setting this flag to Enabled

I cannot reproduce this issue. Are you setting it just to Enabled or one of the other options?

[frankjrp]

I cannot reproduce this issue. Are you setting it just to Enabled or one of the other options?

Yes! I'm setting it just to Enabled.

But it's important to follow these steps:

1. Enable the flag;
2. Relaunch the browser;
3. Close the browser (from the background too);
4. Open Bromite again;
5. Open the menu and see the error.

[hbarsaiyan]

It is fixed now for me. Thank you.

[csagan5]

It is fixed now for me. Thank you.

Nothing has been fixed; you might have changed the flags.

[csagan5]

Fixed in 89.0.4389.92.

[hbarsaiyan]

@csagan5 don't we have an arm32 and arm64 release this time? Just asking.

[Nemris]

@csagan5 Doesn't seem to be solved on Bromite 89.0.4389.92 - setting #tabbed-app-overflow-menu-regroup and restarting Bromite two to three times still yields an empty top row in the overflow menu.

[uazo]

this is what i expect:

what am I doing wrong?

[Nemris]

@uazo In my case, the top button row above New tab is empty when enabling the abovementioned flag. The row itself is still present, but without buttons.

I'm on stock Magisk rooted Android 10, One UI 2.0 on a SM-A405FN.

[uazo]

I checked, it seems that @csagan5 in 89.0.4389.92 has removed (perhaps by mistake) my changes.

in the meantime I've found a bug in Remove-help-menu-item.patch
https://github.com/bromite/bromite/blob/master/build/patches/Remove-help-menu-item.patch#L22

[csagan5]

@uazo not by mistake, I did not add the extra icon from your patch. Is that needed to fix the blank row?

[uazo]

yes, the fix is right there! :)

adding a sixth button in the header of the app menu (you did after #943), a sixth icon have to be added in the row layout used to display, otherwise the switch does not lead to anything and the row is empty (see https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/android/appmenu/internal/java/src/org/chromium/chrome/browser/ui/appmenu/AppMenuAdapter.java;l=174;drc=40f5195e60a82e940f4ef149e55944da9c8bf1f0;bpv=1;bpt=1)

for the same reason I increased the width of the menu by the size of an icon (the other part you removed)

[csagan5]

adding a sixth button in the header of the app menu (you did after #943), a sixth icon have to be added in the row layout used to display, otherwise the switch does not lead to anything and the row is empty

I do not understand how this is related to the patch to bookmark all tabs 🤔

[uazo]

I do not understand how this is related to the patch to bookmark all tabs

uhm... let's see if I can explain it..
do you see these changes?

From here:

since the menu is developed in a sort of mvc pattern, the xml is not really the view but it is the model. the view is generated by AppMenuAdapter in the switch:

since the icons are no longer 5 but 6, the corresponding call createMenuItemRow is missing, so no view is created and the row is empty

[csagan5]

This explanation could have been in the commit message :)

There is a mistake in the text associated to the button, it should have been left as before; I will fix it on next release.

When the flag is disabled or default: both menus have the 'Bookmark all tabs' menu entry
When the flag is enabled: the entry is not present in the tab hover menu and the white bar issue manifests.

Since there are other menu entries in the menu which are not on the top bar, how to fix the behaviour so that it is always like when flag is disabled?

[uazo]

This explanation could have been in the commit message :)

sorry, I thought it was clear with the changes. I'll be more wordy next time :)

When the flag is disabled or default: both menus have the 'Bookmark all tabs' menu entry

what do you mean by both? header row and menu item?
if so, my changes are needed + must be added

bromite/build/patches/Add-menu-item-to-bookmark-all-tabs.patch

Lines 46 to 48 in 167f960

	+ <item android:id="@+id/bookmark_all_tabs_menu_id"
	+ android:title="@string/menu_bookmark_all_tabs"
	+ android:icon="@drawable/ic_folder_blue_24dp" />

in main_menu_regroup.xml

[csagan5]

what do you mean by both? header row and menu item?

No, the regular 3-dots menu and the one after tapping on the tabs counter.

Since there are other menu entries in the menu which are not on the top bar, how to fix the behaviour so that it is always like when flag is disabled?

See my other question ^

[uazo]

do you mean here?

[frankjrp]

do you mean here?

I think @csagan5 is talking about the menu entries as shown below (when the flag is disabled or default):

PS: Since you guys are fixing the menu issues, there is a missing icon after tapping on the tabs counter then the menu.

[Nemris]

Can reproduce the missing Exit icon too.

[csagan5]

do you mean here?

@uazo regardless of the overflow menu regroup flag, the feature should work like this:

• one menu entry with text and icon on the regular three-dots menu (e.g. like History)
• one menu entry with text and icon on the three-dots menu when accessing the tabs hover menu (e.g. like New Incognito Tab)

As @frankjrp shows in the screenshots.

there is a missing icon after tapping on the tabs counter then the menu.

You mean the exit menu icon, right?

[frankjrp]

You mean the exit menu icon, right?

Exactly.

[uazo]

@uazo regardless of the overflow menu regroup flag, the feature should work like this:
one menu entry with text and icon on the regular three-dots menu (e.g. like History)
one menu entry with text and icon on the three-dots menu when accessing the tabs hover menu (e.g. like New Incognito Tab)

removing this icon? (note: I only have it because I applied my patch)

[csagan5]

removing this icon?

That is not sufficient. If the icon is removed, then the behavior is not the same regardless of the flag. The menu entry would be missing.

Here I mean the icon on the left of the text. No changes to the icon-only bar above.

regardless of the overflow menu regroup flag, the feature should work like this:

• one menu entry with text and icon on the regular three-dots menu (e.g. like History)
• one menu entry with text and icon on the three-dots menu when accessing the tabs hover menu (e.g. like New Incognito Tab)

[csagan5]

@uazo shall I assign this to you? Otherwise I can try fixing it

[uazo]

sure, I'll take care of it

[csagan5]

@frankjrp is the exit menu missing icon fixed already?

[frankjrp]

is the exit menu missing icon fixed already?

@csagan5 yes, it's fixed!

[csagan5]

I made a fix for the menu items; will be fixed on next release.

[csagan5]

Fixed in 90.0.4430.59.

[csagan5]

I still see the blank empty item on top.

[frankjrp]

Interesting! For me it's ok since the version 90.0.4430.59

[uazo]

@csagan5 I think it can be closed, can you confirm?

[frankjrp]

@csagan5 I think it can be closed, can you confirm?

@uazo Yes, everything is working fine.

[csagan5]

Fixed in 90.0.4430.59.