1803773 - 'Bookmarks Toolbar' context menuitem cropped on pre-Windows 10 systems

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 109.0a1 20221201161829 on Windows 8.1

The tabs toolbar's menu item 'Bookmarks Toolbar' is cropped. This is no issue in 108.0b8.

Emilio, do you know which change altered this?

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: screenshot: bookmarks toolbar cropped

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1803773 - Remove nsMenuFrame and nsMenuParent.

Move all the event handling stuff to the DOM.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1803773 - Use legacy layout in native menupopups on Windows for now. r=Gijs

D164210 is the "right" fix, but that needs more work and I'm not going
to land that during the soft freeze, so let's use this for now.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/826d104218ac
Use legacy layout in native menupopups on Windows for now. r=Gijs

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/826d104218ac

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1803773 - Use activateItem() rather than click() to activate menuitems. r=Gijs!

Bug 1805414 will move menu event handling to the DOM.

With that change the current synthetic click behavior of XUL menuitems
breaks. On current central, we rely on nsMenuFrame::HandleEvent not
getting called at all for synthetic clicks, and instead we just fire a
command event synchronously here:

https://searchfox.org/mozilla-central/rev/a0d4f8f112c5c792ae272bf6ce50763ddd23ffa2/dom/xul/nsXULElement.cpp#1071

After my patch the command event is fired properly (potentially
asynchronously too) by the regular menu activation machinery, which is
preferable.

• They fire a command event synchronously (even though on some
  platforms like macOS activating a context menu item is async).

• They use a totally different codepath from what a user does.

• They don't deal with native menus, etc.

We have a proper API for this (activateItem) which takes a much more
closer codepath to what users do, requires that the menu is shown, etc.
Use that API instead for testing.

As a benefit, tests now do not need to close the context menu manually
when clicking on a menu item (because we trigger the same code path as
users clicking the menu).

Comment 7

[Phabricator Automation]

Comment on attachment 9307308 [details]
WIP: Bug 1803773 - Remove nsMenuFrame and nsMenuParent.

Revision D164210 was moved to bug 1805414. Setting attachment 9307308 [details] to obsolete.

Comment 8

[Phabricator Automation]

Comment on attachment 9308010 [details]
Bug 1803773 - Use activateItem() rather than click() to activate menuitems. r=Gijs!

Revision D164567 was moved to bug 1805415. Setting attachment 9308010 [details] to obsolete.