941109 - Australis: Overflow&menu panels should respect closemenu xul attribute in children

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Carlos [nohamelin]]

My Simple Locale Switcher extension has the following xul code for a toolbar button:

<toolbarbutton type="menu">
  <menupopup>
    <menuitem type="checkbox" closemenu="none"/>
    ...

If this button is placed inside the new menu panel or the overflow panel, toggling the checkbox keep the button's popup open *but* the parent panel is closed. See:

https://developer.mozilla.org/en-US/docs/XUL/Attribute/closemenu

Comment 1

[Carlos [nohamelin]]

From comment #5 of bug 940693, noautoclose="true" seems to work as I expected of closemenu="none". It seems that closemenu is for panels as closemenu is for menus. Or maybe the old was simply missed? It seems be hardly used.

Comment 2

[:Gijs (he/him)]

(In reply to Carlos [:nohamelin] from comment #1)
> From comment #5 of bug 940693, noautoclose="true" seems to work as I
> expected of closemenu="none". It seems that closemenu is for panels as
> closemenu is for menus. Or maybe the old was simply missed? It seems be
> hardly used.

Yeah, I don't think we realized there was an existing attribute to use for this. As we're still on Nightly and probably won't go to Aurora in 3 weeks already, we should evaluate if we can switch to the existing attribute or if that somehow doesn't make sense. Dão, what do you think?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

We just talked to Dao IRL, and we're going to attempt to support the closemenu attribute in the panel for toolbarbuttons as well as menuitems of the panel's children. This will supplant the noautoclose attribute that we're currently using.

Comment 4

[:Gijs (he/him)]

Attachment: use closemenu instead of noautoclose attribute in Australis menupanel,

I think my editor's search/replace works. The tests agree. Mike, do you, too? :-)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362607 [details] [diff] [review]
use closemenu instead of noautoclose attribute in Australis menupanel,

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

DXR confirms that these are all of the instances of noautoclose being used.

This looks good to me - though we might want to think about supporting closemenu="single", and what that means for things like items in subviews.

::: browser/components/customizableui/content/panelUI.js
@@ +329,5 @@
>     * This function can be used as a command event listener for subviews
>     * so that the panel knows if and when to close itself.
>     */
>    onCommandHandler: function(aEvent) {
> +    if (aEvent.originalTarget.getAttribute("closemenu") != "none") {

What about if closemenu is "single" in this case? Or would you rather support "single" in a follow-up?

Comment 6

[:Gijs (he/him)]

Attachment: use closemenu instead of noautoclose attribute in Australis menupanel,

This adds support for closemenu == 'single' to the command handler. Thinking about it though, I don't really see a usecase for it... but hey, it wasn't hard to do.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362695 [details] [diff] [review]
use closemenu instead of noautoclose attribute in Australis menupanel,

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

This looks fine to me. Tests would be nice, but I'm not going to block on that.

Comment 8

[:Gijs (he/him)]

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

There are some tests in the patch on bug 940307.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/868c2aeb6e55