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

[Bug] New tab three-dot menu should be wider when not signed in #10133

Closed
eliserichards opened this issue Apr 15, 2021 · 10 comments
Closed

[Bug] New tab three-dot menu should be wider when not signed in #10133

eliserichards opened this issue Apr 15, 2021 · 10 comments
Assignees
Milestone

Comments

@eliserichards
Copy link
Contributor

eliserichards commented Apr 15, 2021

image

When we're signed in to sync, the menu is wide enough to see the whole "Desktop Site" string. But when we're signed out, "Desktop site" gets cut off. I think the minimum width should allow us to see that whole string (in En-US)

┆Issue is synchronized with this Jira Task

@eliserichards
Copy link
Contributor Author

Tagging @violasong and @brampitoyo for feedback

@Username1-a
Copy link

Excuse me, but why not keeping current layout from stable Firefox ("Desktop site" string is absent from home/new tab page 3-dot menu since it doesn't have desktop layout)?

And narrower 3-dot menu, as before, I think looks a bit cleaner and easier on the eyes as it doesn't obstruct as much content

"Synced tabs" instead of that long email string looked pretty reasonable, too

@cadeyrn
Copy link
Contributor

cadeyrn commented Apr 16, 2021

"Desktop site" string is absent from home/new tab page 3-dot menu since it doesn't have desktop layout

This is not for a desktop layout of the home screen but to request the desktop version of the website you intend to load in a new tab. For reference: Chrome also shows this option in the home screen menu.

And narrower 3-dot menu, as before, I think looks a bit cleaner and easier on the eyes as it doesn't obstruct as much content

"Synced tabs" instead of that long email string looked pretty reasonable, too

This is both off-topic. This issue is about the minimum width of the menu for users who are not signed in and therefore there is no e-mail-address in the menu. It's also not about the menu layout in general. See mozilla-mobile/fenix#17796 as meta issue for the new menu.

@Mugurell
Copy link
Contributor

The menu is as wide as needed to show the widest menu item (with a maximum set).
Based on the first screenshot the menu can be wider if the "sign-in item" - widest item requests so but in the second screenshot the "desktop site item" - widest item gets cut off. So this seems to be a bug of this menu item which doesn't report the right width.

@Mugurell Mugurell self-assigned this Apr 16, 2021
@sheikh-azharuddin
Copy link

But is it necessary to show the email address at all??
Because of this menu is getting wider

@Mugurell
Copy link
Contributor

I though at first that this might be an Android bug so I've created a toy project in which to test the Android widget that that menu item uses - a SwitchCompat, also in the hope that this could be used to report the issue to Google.
But there all is working as expected.
So although there is almost nothing we do to configure that widget something is impacting the behavior.
And I saw that indeed the width that that item returns is wrong. It does not account for the left drawable so the menu item has space only for the text and the thumb but also layouts that left drawable which pushes the text to the right resulting in it being ellipsized.

@Mugurell
Copy link
Contributor

Mugurell commented Apr 22, 2021

This was a bug for which many planets had to be perfectly aligned:

  • Because of the menu is highly dynamic - has a dynamic width (from DynamicWidthRecyclerView) and a dynamic height (from ExpandableLayout) it has to measure and even layout items even before having a Window in which to show them (this happening later in BrowserMenu).
  • This Desktop site menu item is a SwitchCompat .. a TextView witch allows to set compound drawables and we do with that left desktop drawable.
  • We are not setting a layout direction (hence the "inherit" default will be used) but are using relative positioning for that drawable.
  • Because the layout direction is inherit but the item is not yet attachedToWindow() the inheritance chain is not complete and the layout direction cannot be resolved.
    If we'd set a different layout direction or used drawableLeft instead of drawableStart this bug would not happen.
  • TextView has an interesting behavior: it will not show these drawables if they are set with a relative direction (as in dynamic layout direction (LTR / RTL)) and that direction cannot be resolved, that being checked in View#resolveDrawables.
  • So the onMeasure() of TextView will not add the size and padding of the relative drawables if they are not showing (the drawable is available in TextView but does not exist in the mShowing array)

All this will lead to the item returning a smaller width to DynamicWidthRecyclerView.
But eventualy the item will be added to a Window and all these checks will ran again, this time all working as expected so that left drawable will be shown but in a menu which has been setup with a smaller width.

@Mugurell
Copy link
Contributor

Moving the ticket to AC where I'll open a PR to fix it.

@Mugurell Mugurell transferred this issue from mozilla-mobile/fenix Apr 22, 2021
Mugurell added a commit to Mugurell/android-components that referenced this issue Apr 22, 2021
…right dimensions

A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
Mugurell added a commit to Mugurell/android-components that referenced this issue Apr 27, 2021
…right dimensions

A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
mergify bot pushed a commit that referenced this issue Apr 27, 2021
A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
@Mugurell
Copy link
Contributor

Moving the ticket from Fenix to AC wasn't the best choice.
We still need a way to track issues and verify them on Fenix, so I've created a new ticket there.

eliserichards pushed a commit to eliserichards/android-components that referenced this issue Apr 29, 2021
…right dimensions

A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
mergify bot pushed a commit that referenced this issue Apr 29, 2021
A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.

(cherry picked from commit a0a9315)

# Conflicts:
#	docs/changelog.md
eliserichards pushed a commit to eliserichards/android-components that referenced this issue Apr 29, 2021
…right dimensions

A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
mergify bot pushed a commit that referenced this issue Apr 29, 2021
A BrowserMenuCompoundButton setup with compound drawables and used in our
BrowserMenu configured with a DynamicWidthRecyclerVIew will return a width
smaller with the size + padding of the compound drawables.

By replacing the default `inherit` layout direction for the initial onMeasure
we ensure the menu will also count the bounds of the compound drawables.
@Mugurell
Copy link
Contributor

This was verified on Fenix - mozilla-mobile/fenix#19310 (comment)
Moving to Done.

@gabrielluong gabrielluong added this to the 90.0.0 🌳 milestone May 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants