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

RESOLVED INVALID

Status

()

defect
RESOLVED INVALID
6 years ago
Last year

People

(Reporter: Manuel.Spam, Assigned: bidahor13, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(2 attachments)

Reporter

Description

6 years ago
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

6 years ago
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?
Flags: needinfo?(Manuel.Spam)
Whiteboard: [Australis:P4]
Reporter

Comment 2

6 years ago
Posted file 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...
Flags: needinfo?(Manuel.Spam)

Comment 3

6 years ago
(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?
Flags: needinfo?(Manuel.Spam)
Reporter

Comment 4

6 years ago
(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...
Flags: needinfo?(Manuel.Spam)

Comment 5

6 years ago
(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.

Updated

6 years ago
Depends on: 948213

Comment 6

5 years ago
This actually seems to work now, with this test add-on? Manuel, can you confirm?
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(Manuel.Spam)
Resolution: --- → WORKSFORME
Reporter

Comment 7

5 years ago
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.
Flags: needinfo?(Manuel.Spam)

Comment 8

5 years ago
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.
Status: RESOLVED → REOPENED
Flags: firefox-backlog+
Resolution: WORKSFORME → ---

Comment 9

5 years ago
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!)
Flags: needinfo?(mhoye)
Whiteboard: [Australis:P4] → [good first bug][mentor=Gijs][lang=js]
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.
Flags: needinfo?(mhoye)
Mentor: gijskruitbosch+bugs
Whiteboard: [good first bug][mentor=Gijs][lang=js] → [good first bug][lang=js]

Comment 11

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

Comment 12

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

Comment 13

5 years ago
(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

5 years ago
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?
Tejdeepg, are you still working on this? If not, I'll ask Srimanta to take a look.

Updated

5 years ago
Whiteboard: [good first bug][lang=js] → [good next bug][lang=js]
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.
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?
Flags: needinfo?(Manuel.Spam)

Comment 18

5 years ago
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 :-) )
Assignee: nobody → swapnil.rp15
Flags: needinfo?(Manuel.Spam)
Thanks Gijs. Sure, will needinfo you. Thanks for clearing my dilemma. :-)
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

4 years ago
(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

4 years ago
Hey Swapnil, how are you getting on here? Do you need more help to work this one out?
Flags: needinfo?(swapnil.rp15)

Comment 23

3 years ago
Looks like Swapnil is no longer working on this.
Assignee: swapnil.rp15 → nobody
Flags: needinfo?(swapnil.rp15)
Assignee

Comment 24

3 years ago
Hi Gijs,

Can I work on this bug with you?

Comment 25

3 years ago
(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.
Assignee: nobody → bidahor13

Comment 26

3 years ago
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!
We got rid of the panel as well as legacy add-ons, so can't do anything here anymore.
Status: REOPENED → RESOLVED
Closed: 5 years agoLast year
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.