Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

[Bug] Browser menus can be too narrow #9922

Closed
Mugurell opened this issue Mar 15, 2021 · 3 comments
Closed

[Bug] Browser menus can be too narrow #9922

Mugurell opened this issue Mar 15, 2021 · 3 comments
Assignees
Labels
🐞 bug Something isn't working <menu> Component: concept-menu, browser-menu, browser-menu2 MR1 Issues that are needed for the MR1 2021 release.
Milestone

Comments

@Mugurell
Copy link
Contributor

Mugurell commented Mar 15, 2021

Seen while testing mozilla-mobile/fenix#17772
With newCoreMenuItems the menu may be too narrow.

Currently seems to only reproduce only when the toolbar is set on top:

MenuIsTooNarrow.mp4

┆Issue is synchronized with this Jira Task

@Mugurell
Copy link
Contributor Author

Looked into this a bit, seems like something that can easily be fixed in AC.

@Mugurell Mugurell transferred this issue from mozilla-mobile/fenix Mar 17, 2021
@Mugurell Mugurell self-assigned this Mar 17, 2021
@Mugurell Mugurell changed the title [Bug] [Debug] Browser menus are too narrow [Bug] Browser menus can be too narrow Mar 17, 2021
@Mugurell
Copy link
Contributor Author

This is a very interesting issue of the DynamicWidthRecyclerView that allows the menu to have a dynamic width (with some safe bounds) happening when there are many items to be displayed.

The RecyclerView is a bit too dynamic, in that it will always have the width of the maximum displayed children. This means that depending on the parent and LayoutParams, with enough items to scroll through it could potentially yo-yo in width depending in what's displayed at that exact moment.

I think this is a bit unexpected and what we'd want is to have it have the width of the largest item.

Saw that in the first onMeasure the width will be reported correctly, because all children are accounted, but then in the first layout phase detachAndScrapAttachedViews will be used to limit how many items will actually be laid out depending on if they fit the screen. And this is done by actually removing the child views so if we try to measure the list again, there are fewer children to measure and we might get different dimensions.

To accommodate our needs seems like we should just cache the first reported width and only use that afterwards.

@Mugurell Mugurell added the MR1 Issues that are needed for the MR1 2021 release. label Mar 17, 2021
@Mugurell Mugurell added <menu> Component: concept-menu, browser-menu, browser-menu2 🐞 bug Something isn't working labels Mar 17, 2021
Mugurell added a commit to Mugurell/android-components that referenced this issue Mar 17, 2021
Previously the width could've change while the menu was scrolled or the width
may have been too small / big for the entire list when a larger / narrower
would be initially off screen, waiting for a scroll to be laid out.
Mugurell added a commit to Mugurell/android-components that referenced this issue Apr 2, 2021
Previously the width could've change while the menu was scrolled or the width
may have been too small / big for the entire list when a larger / narrower
would be initially off screen, waiting for a scroll to be laid out.
mergify bot pushed a commit that referenced this issue Apr 2, 2021
Previously the width could've change while the menu was scrolled or the width
may have been too small / big for the entire list when a larger / narrower
would be initially off screen, waiting for a scroll to be laid out.
@abodea
Copy link
Member

abodea commented Apr 7, 2021

Verified as fixed on the latest Nightly and Debug builds with Google Pixel 4 XL (11) for more details please check the following:

Device Build Reproducible?
Google Pixel 4 XL 11 Nightly 4/7/21 No
Google Pixel 4 XL 11 Nightly 4/6/21 No
Google Pixel 4 XL 11 Debug 4/7/21 (latest Master) No
Google Pixel 4 XL 11 Debug 3/30/21 Yes

Note that the latest mentioned try on the table is with a version of Master before the merges.
Others
Screenshot_20210407-144712
Debug 3/30/21
Screenshot_20210407-151758

@abodea abodea closed this as completed Apr 7, 2021
@gabrielluong gabrielluong added this to the 75.0.0 milestone Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Something isn't working <menu> Component: concept-menu, browser-menu, browser-menu2 MR1 Issues that are needed for the MR1 2021 release.
Projects
None yet
Development

No branches or pull requests

3 participants