964944 - Opening "menulist", placed inside "toolbaritem" on panel/panelview, closes panel/panelview

Status: RESOLVED INVALID

(Firefox :: Theme, defect)

Description

[Manuel Reimer]

In my addon "PrefBar" I use the "menulist" element wrapped inside a "toolbaritem". If this combination is placed to either the "panel" or to a "panelview", then popping up the "menulist" closes panel or panelview.

In the past there was a "noautoclose" attribute to disable such effects, but this seems to be gone. So please either "whitelist" the menulist item or reintroduce something like the "noautoclose" attribute.

Comment 1

[:Gijs (he/him)]

The replacement for noautoclose is "closemenu", see https://developer.mozilla.org/en-US/docs/XUL/Attribute/closemenu .

I'm not familiar with <menulist>... can you provide a testcase (scratchpad) or link to a simplified test addon?

Comment 2

[Manuel Reimer]

Attachment: testaddon.xpi

Simple test addon. If you place the button, created by this addon, onto a toolbar, then you can see, that opening the menulist closes the panelview.

It doesn't close if you place the button to the "PanelUI" menu of Firefox but it also doesn't close if I click on "toolbarbutton" elements, there. Seems to be another bug where I have to find a solution for...

Comment 3

[:Gijs (he/him)]

(In reply to Manuel Reimer from comment #2)
> Created attachment 8367545 [details]
> testaddon.xpi
> 
> Simple test addon. If you place the button, created by this addon, onto a
> toolbar, then you can see, that opening the menulist closes the panelview.

Great, thank you! I hope to have time to look into this later this week or possibly next week.

> It doesn't close if you place the button to the "PanelUI" menu of Firefox
> but it also doesn't close if I click on "toolbarbutton" elements, there.
> Seems to be another bug where I have to find a solution for...

This is much more worrying to me, and I am not sure I understand you correctly. Are you saying the panel menu isn't closing at all when you click a toolbar button element? Even some of the ones that are there by default? Does this happen on a clean profile and/or can you provide more detailed STR for that?

Comment 4

[Manuel Reimer]

(In reply to :Gijs Kruitbosch from comment #3)
> This is much more worrying to me, and I am not sure I understand you
> correctly. Are you saying the panel menu isn't closing at all when you click
> a toolbar button element? Even some of the ones that are there by default?
> Does this happen on a clean profile and/or can you provide more detailed STR
> for that?

This only happens if I click on one of the "toolbarbutton" elements in the "panelview", added by my test addon. Seems like I've missed something there...

Comment 5

[:Gijs (he/him)]

(In reply to Manuel Reimer from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > This is much more worrying to me, and I am not sure I understand you
> > correctly. Are you saying the panel menu isn't closing at all when you click
> > a toolbar button element? Even some of the ones that are there by default?
> > Does this happen on a clean profile and/or can you provide more detailed STR
> > for that?
> 
> This only happens if I click on one of the "toolbarbutton" elements in the
> "panelview", added by my test addon. Seems like I've missed something
> there...

For now, yes, but you shouldn't need to do anything there. I'm working to fix that bit in bug 948213.

Comment 6

[:Gijs (he/him)]

This actually seems to work now, with this test add-on? Manuel, can you confirm?

Comment 7

[Manuel Reimer]

If I use "closemenu", then the panel no longer closes. So this part is fixed.

I don't know what is the expected behaviour for "toolbaritem" items in the new style menus and how far they are officially supported.

If I use "noautoclose", then I also have to get sure to close the panel manually after the user selected from a "menulist" embedded in a "toolbaritem" item. This is what I would call a workaround or "hack".

The best possible behaviour would be that no "noautoclose" is needed and the backend knows that opening a menulist should not close the panel but it should be closed after the user did a selection in the list.

Comment 8

[:Gijs (he/him)]

Ah, so I assumed that your test add-on reflected the problem, but you'd already added closemenu="none" attributes. Let's reopen this, then. We should be able to get this right.

Comment 9

[:Gijs (he/him)]

In order to fix this bug, what needs to happen is:

- http://hg.mozilla.org/mozilla-central/annotate/a7d685480bf2/browser/components/customizableui/src/CustomizableUI.jsm#l1376 needs to be adjusted to also be true if tagName == "menulist"
- we need to add a condition before http://hg.mozilla.org/mozilla-central/annotate/a7d685480bf2/browser/components/customizableui/src/CustomizableUI.jsm#l1427 that returns |true| iff target.localName == "menulist"
- we need to add an add_task(...) test in http://hg.mozilla.org/mozilla-central/file/a7d685480bf2/browser/components/customizableui/test/browser_940307_panel_click_closure_handling.js that creates a new menulist item (inside a <toolbaritem> !) with a few options, adds it to the panel, and checks that we don't close the panel if clicking the menulist, but do close it when selecting an item from the options inside the menu. To see how to do this, you can use the other tests in the file.

Please don't hesitate to ask me for help using the "need more information" flag below the comment field.


(Mike: can you let me know if you agree that this works for a 'good first bug' or if this needs to be 'good next bug', too? Thanks!)

Comment 10

[Mike Hoye [:mhoye]]

This is a pretty good good-first, yeah! Concise description of what we need, links to where the changes need to be made, I'll take it.

Comment 11

[tejdeepg]

Would like to pick up this bug. Please assign it to me.

Comment 12

[tejdeepg]

Would like to pick up this bug. Please assign it to me.

Comment 13

[:Gijs (he/him)]

(In reply to tejdeepg from comment #12)
> Would like to pick up this bug. Please assign it to me.

Hi tejdeepg, that sounds great. We don't normally assign bugs for people contributing their first patch before they provide that patch. So please let me know if you need any more help beyond the instructions in comment #9, and I look forward to seeing your patch! :-)

Comment 14

[Srimanta Barua]

Hi. I'd like to take this up as a first bug. But I'm not sure about what we have to do for the third line of the instructions in comment #9. Could you please help me with that?

Comment 15

[Mike Hoye [:mhoye]]

Tejdeepg, are you still working on this? If not, I'll ask Srimanta to take a look.

Comment 16

[Swapnil R Patil [:swaprp]]

Hi,

I would like to work on this. But I have one doubt in reproducing this issue. Please see the comments for the attached image.

Thanks.

Comment 17

[Swapnil R Patil [:swaprp]]

Attachment: Screenshot to clear the doubt

Is the area with green box called PanelUI?
Is the area with red box called toolbar?
What is panelview - is this the view we get when we click on the addon button?

Comment 18

[:Gijs (he/him)]

No, the green area is also a toolbar. "PanelUI" is the panel that comes up when you click the button with the three horizontal lines ("hamburger" button) on the far right of the toolbar (directly right of the green-highlighted area in your image).

(in future, please needinfo me, not manuel :-) )

Comment 19

[Swapnil R Patil [:swaprp]]

Thanks Gijs. Sure, will needinfo you. Thanks for clearing my dilemma. :-)

Comment 20

[Swapnil R Patil [:swaprp]]

Hi Gijs,

I added the first two changes you mentioned but seems it didn't fix the issue.

I am assuming that the issue is - if we add addon button to the PanelUI and select any of the entry in the dropdown list then the panel should be closed (just like it closes when we select a button or checkbox in it). Am I right?

Comment 21

[:Gijs (he/him)]

(In reply to Swapnil R Patil [:swaprp] from comment #20)
> Hi Gijs,
> 
> I added the first two changes you mentioned but seems it didn't fix the
> issue.
> 
> I am assuming that the issue is - if we add addon button to the PanelUI and
> select any of the entry in the dropdown list then the panel should be closed
> (just like it closes when we select a button or checkbox in it). Am I right?

No, the issue is that if we add a menulist, and you open the dropdown list from the <menulist> element, the panel should stay open. It should probably close when selecting an item. You should be able to test with the test addon attached to the bug.

Comment 22

[:Gijs (he/him)]

Hey Swapnil, how are you getting on here? Do you need more help to work this one out?

Comment 23

[:Gijs (he/him)]

Looks like Swapnil is no longer working on this.

Comment 24

[bidahor13]

Hi Gijs,

Can I work on this bug with you?

Comment 25

[:Gijs (he/him)]

(In reply to bidahor13 from comment #24)
> Hi Gijs,
> 
> Can I work on this bug with you?

Yes! The links in comment 9 should be helpful in figuring out what needs happening. I'll get you some updated ones in a sec.

Comment 26

[:Gijs (he/him)]

Right, so... the guts of this change will go in this method:

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/browser/components/customizableui/CustomizableUI.jsm#1529-1535

(that's the comment at the top - the method is quite long...)

It's basically trying to tell for any click event "did the user just click on something for which we should keep the panel open, or on something else (and then we'll close the panel)".

We need to teach that code about menulists, which are commonly called "combo boxes" - in HTML they'd be a <select size=1>, like here: http://jsbin.com/sevawirupi/edit?html,output

Obviously, when the user clicks the menulist (to show the items in it), the menu should stay open. When the user then selects an item in the menulist, we should close the panel.

So to do this, I *think* the easiest way is to change this line:

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/browser/components/customizableui/CustomizableUI.jsm#1586

to also set inItem to true if tagName == "menulist".

That way, we'll bail out of the loop here:

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/browser/components/customizableui/CustomizableUI.jsm#1606-1610

and the 'target' variable will be set to the menulist.

Then you can add an if block before this one:

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/browser/components/customizableui/CustomizableUI.jsm#1632-1636

that checks if we're inItem and if the target's tagName is 'menulist', and if so, return true.

If you've done that and rebuild your tree with "./mach build faster" (do ask me or ping people in #introduction on IRC if you don't have your Firefox build set up yet!), you should be able to use Manuel's test add-on (attached to this bug) to check that it works correctly.

The final step would be adding a testcase for this issue to an existing test file, but we'll get to that - one step at a time.

Let me know if you have any other questions or need more help!

Comment 27

[:Gijs (he/him)]

We got rid of the panel as well as legacy add-ons, so can't do anything here anymore.