Closed Bug 1763910 Opened 3 months ago Closed 2 months ago

Main menu items can be cut off, if the localized "Zoom" text is long-ish and when there's a scrollbar

Categories

(Firefox :: Toolbars and Customization, defect, P3)

defect

Tracking

()

VERIFIED FIXED
101 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox99 --- unaffected
firefox100 --- verified
firefox101 --- verified

People

(Reporter: itiel_yn8, Assigned: sfoster)

References

(Regression)

Details

(Keywords: nightly-community, regression, reproducible, Whiteboard: [fidefe-2022-mr1-css-linting])

Attachments

(2 files)

STR:

  1. Be on a certain monitor configuration (small monitor size / high DPI etc) that would make the main menu scrollable
  2. Have a localized version of Nightly that the "Zoom" text on the main menu would be a bit long (or, alternatively, replace the value="Zoom" attribute on the Zoom's label to value="מרחק מתצוגה" (Hebrew), or value="ზომის ცვლილება" (Georgian))
  3. Open the main menu

AR:
Menu items are readable and not cut off.

ER:
In the Hebrew case, the menu items are still visible but hovering them makes it obvious something is cutoff from the side.
In the extreme case in Georgian, the shortcuts are almost emtirely cut off.

Attached image Screenshot

Set release status flags based on info from the regressing bug 1762148

:sfoster, since you are the author of the regressor, bug 1762148, could you take a look?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sfoster)
Has Regression Range: --- → yes
Severity: -- → S3
Priority: -- → P3

Thanks, I'll look into this.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(sfoster)

This is because the .toolbaritem-combined-buttons:is([cui-areatype="menu-panel"], [overflowedItem=true]) selector I'm using in panelUI-shared.css doesnt match these items when they are in the main menu. There's no cui-areatype attribute on the items in this menu. So, I'm not sure if the best solution is to add/revert to the panelview .toolbaritem-combined-buttons we were using previously (which remember doesnt match these button when they are in the overflow menu in customize mode.) or something else - maybe ensure mainMenu toolbar items do have a cui-areatype attribute?

Flags: needinfo?(dao+bmo)

For the purpose of uplifting, let's revert to :is(:not([cui-areatype="toolbar"]), [overflowedItem=true]). But then I still think these attributes are pretty unhelpful for CSS (which I think is their main purpose?), so going forward I think we should either figure out how to wrap the customize mode fake panel in a panelview element so we can use that selector, or otherwise revamp / consolidate cui-areatype in a way that makes sense (maybe fold overflowedItem into it as well). Gijs, do you have any further thoughts?

Flags: needinfo?(dao+bmo) → needinfo?(gijskruitbosch+bugs)

(In reply to Dão Gottwald [::dao] from comment #6)

For the purpose of uplifting, let's revert to :is(:not([cui-areatype="toolbar"]), [overflowedItem=true]). But then I still think these attributes are pretty unhelpful for CSS (which I think is their main purpose?), so going forward I think we should either figure out how to wrap the customize mode fake panel in a panelview element so we can use that selector, or otherwise revamp / consolidate cui-areatype in a way that makes sense (maybe fold overflowedItem into it as well). Gijs, do you have any further thoughts?

cui-areatype is/was also used for logic, and is meant to indicate the CustomizableUI area type that something is in (a decision from the australis / fx29 days). This meant that items that overflow from the toolbar into the overflow menu have areatype=toolbar, whereas items that were in the then-customizable main menupanel had areatype=panel. When for Photon we made the main hamburger menu non-customizable, but made the overflow panel customizable, the areatype=panel bit migrated there, which led to the confusing situation where:

  1. items in the overflow panel can be either areatype=panel or toolbar, depending on whether they are "pinned" to the overflow panel or not
  2. items in the hamburger menu have no areatype attribute at all

I think in the short term, the panelview descendant selector and making those work in customize mode is the sensible option. For future ease of maintenance it probably makes sense to more clearly split out any logic relating to area types on the one hand, and attributes/classes around "is this in a panel" on the other. Perhaps the areatype attribute should be renamed in some way.

Hopefully that all makes sense...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(sfoster)
See Also: → 1764832

Yeah that makes sense. I've opened bug 1764832 for the panelview follow-up - which seems to be the most sane first step here, once we've got the issue patched on beta.

Flags: needinfo?(sfoster)
Whiteboard: [fidefe-2022-mr1-css-linting]
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/053ec80f1a57
Use :not([cui-areatype=toolbar]) to ensure the combined toolbaritem buttons pick up the right styles when in the hamburger menu. r=dao
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch

Comment on attachment 9272524 [details]
Bug 1763910 - Use :not([cui-areatype=toolbar]) to ensure the combined toolbaritem buttons pick up the right styles when in the hamburger menu. r?Dao!

Beta/Release Uplift Approval Request

  • User impact if declined: The edge of the main menu with the keyboard shortcuts may be cut off for users on low display resolutions where the main menu scrolls.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0. Step 2 (localized version with longer labels) isn't strictly necessary to reproduce.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Well contained patch, restoring original (v99) styles for the combined buttons.
  • String changes made/needed: None
Attachment #9272524 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

We managed to reproduce this issue with Fx 101.0a1 (2022-04-09) (64-bit) on Windows 10 and Windows 10ARM.
Verified fixed with Fx 101.0a1 (2022-04-19) on the same OSs.

Comment on attachment 9272524 [details]
Bug 1763910 - Use :not([cui-areatype=toolbar]) to ensure the combined toolbaritem buttons pick up the right styles when in the hamburger menu. r?Dao!

Approved for 100.0b9

Attachment #9272524 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified fixed with Fx 100.0b9 (treeherder build) on Windows 10 and Windows 10ARM.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.