1790920 - Remove nsButtonBoxFrame (create XULButtonElement or so)

Status: RESOLVED FIXED

(Core :: XUL, task)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1790920 - Remove nsButtonBoxFrame. r=smaug

Create XULButtonElement instead to do the event handling. Pretty much a
straight port, this allows these elements to respect CSS display
properly (and use modern flexbox rather than old XUL layout).

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1790920 - Don't check defaultPrevented for click events closing panels. r=mconley,Gijs,mak

(To be landed right after the soft freeze is over)

One interesting consequence of my previous patch in this bug, is that
the time at which the XUL element processes the click event is different
(earlier, at bubbling phase rather than after everything).

This caused some test failures caused by the panel autohide, since now
it goes after than the native handling and the native handling
default-prevents to dispatch the XUL command.

Given we don't check key events for defaultPrevented either, it makes
sense to be consistent.

This uncovered an interesting failure, where the "show more tabs" button
in the sync menu starts closing the panel unexpectedly on click. The
right fix is to use closemenu="none" in that button, since otherwise
that button is not keyboard accessible (this is an issue on current
nightly that this patch fixes).

Depends on D157509

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1790920 - Propagate modifiers properly in PanelMultiView's key event handling. r=mconley,Gijs,mak

This is needed to fix browser_ext_browserAction_click_types.js.

Before the first patch in this bug, the order of the built-in command
event and the synthetic event here was slightly different, so it was
papered over.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1790920 - Minor fixes to toolbar key navigation. r=mak,Gijs,mconley

This is the last fix needed for this.

The issue is that command events now are triggered by synthetic click
events (which was not the case before pretty much by chance, a side
effect of how these events were implemented).

If we open a panel by a command event triggered by the synthetic click
event, rather than the real keypress event, we end up not detecting that
it is really a keyboard activation, and not focusing the first navigable
element in the panel for example, which is unfortunate.

This was caught by browser_toolbarButtonKeyPress.js.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/581bd16576a0
Remove nsButtonBoxFrame. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c152a51ae80c
Don't check defaultPrevented for click events closing panels. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1bb0cca45dad
Propagate modifiers properly in PanelMultiView's key event handling. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d43a9bd44619
Minor fixes to toolbar key navigation. r=Gijs,extension-reviewers,willdurand

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/581bd16576a0
https://hg.mozilla.org/mozilla-central/rev/c152a51ae80c
https://hg.mozilla.org/mozilla-central/rev/1bb0cca45dad
https://hg.mozilla.org/mozilla-central/rev/d43a9bd44619

Comment 8

[Julien Wajsberg [:julienw]]

Hey,
from mozregression, I believe this broke the (i) button within the profiler popup.

STR:

1. If that's not done yet, add the profiler button by going to profiler.firefox.com and click the big blue button in the middle of the page.
2. Open the profiler popup by clicking the arrow next to the profiler button.
3. Click to the top right (i) button. It should trigger the display of the header of this popup.

But this doesn't seem to work anymore since this patch. Instead the popup is closed.

Could you please have a look?

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Screencast_20220920_155834.webm

Comment 10

[Emilio Cobos Álvarez (:emilio)]

That was a pre-existing issue when activating that button with the keyboard. Filed bug 1791691 to track it.