Closed Bug 934532 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [Australis:P3][Australis:M?]
Blocks: australis-cust
No longer blocks: australis
This was simpler than I thought.
Attachment #832878 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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).
Attachment #832878 - Flags: review?(jaws) → review-
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.
Attachment #8333550 - Flags: review?(jaws)
Attachment #832878 - Attachment is obsolete: true
Attachment #8333550 - Flags: review?(jaws) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/a549ddceda0a
Whiteboard: [Australis:P3][Australis:M?] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a549ddceda0a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.