934532 - Clicking on a blank area in the Menu Panel should not close the menu panel

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Add an extra button to the menu panel so that there are two blank spaces at the bottom of the menu panel.
Click on one of the blank spaces.

ER:
The menu panel should not close.

AR:
The menu panel closes.

I don't see why we would want the menu panel to close in this case, and the current Firefox application menu doesn't close when the blank area beneath the Help button is clicked.

Comment 1

[:Gijs (he/him)]

Attachment: clicking in a blank area of the panel shouldn't close it,

This was simpler than I thought.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 832878 [details] [diff] [review]
clicking in a blank area of the panel shouldn't close it,

Review of attachment 832878 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1080,5 @@
>     */
>    _isOnInteractiveElement: function(aEvent) {
>      let target = aEvent.originalTarget;
> +    let panel = this._getPanelForNode(aEvent.currentTarget);
> +    let inInput = false, inMenu = false, inItem = false;

Our convention is to place these on their own line.

@@ +1082,5 @@
>      let target = aEvent.originalTarget;
> +    let panel = this._getPanelForNode(aEvent.currentTarget);
> +    let inInput = false, inMenu = false, inItem = false;
> +    while (!inInput && !inMenu && !inItem && target != panel) {
> +      let name = target.localName;

s/name/tagName/

@@ +1088,2 @@
>        inMenu = target.type == "menu";
> +      inItem = inItem || name == "toolbaritem" || name == "toolbarbutton";

This doesn't make much sense. Once inItem becomes true, it will never lose its truthiness. But the while loop above only enters this block if inItem is false. I think maybe you meant to reference a different variable here (or you can just drop the `inItem || ` part of the expression since it is guaranteed to be false).

Comment 3

[:Gijs (he/him)]

Attachment: clicking in a blank area of the panel shouldn't close it,

Yeah, I added the \!inItem check to the loop condition as an optimization, and forgot to remove the or. Fixed now. Also, I disagree with our no-joined-declarations policy, but I guess I will have to live with that.

Comment 4

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/a549ddceda0a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a549ddceda0a