958869 - Stop showing toolbar context menu on Australis widgets panels

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Tim Nguyen :ntim]

When you right click the Web Developer, character encoding and History panels.
It will show the toolbar context menu.
Note that this does NOT affect the subviews.

Comment 1

[:Gijs (he/him)]

I don't understand why this should be P1. P4, maybe, and even that seems on the high side...

Comment 2

[u428464]

I don't understand what the issue is here. It was a decision to implement a contextual menu for widgets.

Comment 3

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> I don't understand what the issue is here. It was a decision to implement a
> contextual menu for widgets.

I assumed that the context menu is showing when right clicking the panel shown by these widgets, but I'm not sure. Tim, is that correct?

Comment 4

[u428464]

I've just tested it and it seems to be the issue indeed.

Comment 5

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Guillaume C. [:ge3k0s] from comment #2)
> > I don't understand what the issue is here. It was a decision to implement a
> > contextual menu for widgets.
> 
> I assumed that the context menu is showing when right clicking the panel
> shown by these widgets, but I'm not sure. Tim, is that correct?

Yes, exactly.

Comment 6

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #3)
> I assumed that the context menu is showing when right clicking the panel
> shown by these widgets, but I'm not sure.
I can't reproduce this on nightly 29.0a1 (2014-01-29), win 7, os x 10.8.
Tim?

Comment 7

[Tim Nguyen :ntim]

(In reply to Paul Silaghi, QA [:pauly] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #3)
> > I assumed that the context menu is showing when right clicking the panel
> > shown by these widgets, but I'm not sure.
> I can't reproduce this on nightly 29.0a1 (2014-01-29), win 7, os x 10.8.
> Tim?

It happens with the developer, character encoding and history widgets (in panel of course).

Comment 8

[Paul Silaghi, QA [:pauly]]

1. left click on the developer widget
2. right click on the developer subview
actual results: nothings happens
What am I missing here ?

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Tim Nguyen [:ntim] from comment #7)
> (In reply to Paul Silaghi, QA [:pauly] from comment #6)
> > (In reply to :Gijs Kruitbosch from comment #3)
> > > I assumed that the context menu is showing when right clicking the panel
> > > shown by these widgets, but I'm not sure.
> > I can't reproduce this on nightly 29.0a1 (2014-01-29), win 7, os x 10.8.
> > Tim?
> 
> It happens with the developer, character encoding and history widgets (in
> panel of course).

Could you attach a screen-shot, I'm also not understanding the problem.

Comment 10

[:Gijs (he/him)]

Attachment: panel-contextmenu.png

Comment 11

[:Gijs (he/him)]

(In reply to Paul Silaghi, QA [:pauly] from comment #8)
> 1. left click on the developer widget
> 2. right click on the developer subview
> actual results: nothings happens
> What am I missing here ?

You should have moved the widget to the toolbar first - as comment #0 notes, this doesn't affect subviews, just panels.

Comment 12

[Paul Silaghi, QA [:pauly]]

(In reply to :Gijs Kruitbosch from comment #11)
> You should have moved the widget to the toolbar first
This is what I've missed. Thanks.
Confirmed nightly 29.0a1 (2014-01-29), win 7 x64

Comment 13

[u428464]

As strange as it sounds the current behaviour in the menu bar is the same. Example : right click the history menu in the menu bar.

Comment 14

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #13)
> As strange as it sounds the current behaviour in the menu bar is the same.
> Example : right click the history menu in the menu bar.

Yep, for all submenus (except the bookmark one that creates it's own context menu)

Comment 15

[:Gijs (he/him)]

Attachment: don't show Australis button context menu on subviews or the menubar,

This should do the trick.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8372705 [details] [diff] [review]
don't show Australis button context menu on subviews or the menubar,

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

::: browser/base/content/browser.xul
@@ +508,5 @@
>  #endif
>  #endif
>               context="toolbar-context-menu">
>        <toolbaritem id="menubar-items" align="center"
> +                   context=""

I don't this this is the right thing to do. I think the harm of removing these outweighs the cost of the menu showing with the two disabled items.

Imagine a user who wants to hide their menu bar: A logical thing to do is to right-click on it and toggle it off. This breaks that IMO straightforward flow.

I would consider it a regression to remove of the context menu from the menu bar as it was their prior to Australis.

::: browser/components/customizableui/content/panelUI.js
@@ +295,5 @@
>        tempPanel.setAttribute("type", "arrow");
>        tempPanel.setAttribute("id", "customizationui-widget-panel");
>        tempPanel.setAttribute("class", "cui-widget-panel");
>        tempPanel.setAttribute("level", "top");
> +      tempPanel.setAttribute("context", "");

r+ on this hunk alone.

Comment 17

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #16)
> Comment on attachment 8372705 [details] [diff] [review]
> don't show Australis button context menu on subviews or the menubar,
> 
> Review of attachment 8372705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.xul
> @@ +508,5 @@
> >  #endif
> >  #endif
> >               context="toolbar-context-menu">
> >        <toolbaritem id="menubar-items" align="center"
> > +                   context=""
> 
> I don't this this is the right thing to do. I think the harm of removing
> these outweighs the cost of the menu showing with the two disabled items.
> 
> Imagine a user who wants to hide their menu bar: A logical thing to do is to
> right-click on it and toggle it off. This breaks that IMO straightforward
> flow.
> 
> I would consider it a regression to remove of the context menu from the menu
> bar as it was their prior to Australis.

Huh, interesting. I didn't realize we always did this. I'm still not convinced showing it on the menu items is useful, but anyhow, landed with just the tempPanel fix:

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

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 19

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Matthew N. [:MattN] from comment #16)
> > Comment on attachment 8372705 [details] [diff] [review]
> > don't show Australis button context menu on subviews or the menubar,
> > 
> > Review of attachment 8372705 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/base/content/browser.xul
> > @@ +508,5 @@
> > >  #endif
> > >  #endif
> > >               context="toolbar-context-menu">
> > >        <toolbaritem id="menubar-items" align="center"
> > > +                   context=""
> > 
> > I don't this this is the right thing to do. I think the harm of removing
> > these outweighs the cost of the menu showing with the two disabled items.
> > 
> > Imagine a user who wants to hide their menu bar: A logical thing to do is to
> > right-click on it and toggle it off. This breaks that IMO straightforward
> > flow.
> > 
> > I would consider it a regression to remove of the context menu from the menu
> > bar as it was their prior to Australis.
> 
> Huh, interesting. I didn't realize we always did this. I'm still not
> convinced showing it on the menu items is useful, but anyhow, landed with
> just the tempPanel fix:
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/b07709c0ea92

I think it's useful when clicking on File/Edit/View/...
But not on the menus themselves.

Comment 20

[Matthew N. [:MattN]]

(In reply to Tim Nguyen [:ntim] from comment #19)
> I think it's useful when clicking on File/Edit/View/...
> But not on the menus themselves.

It isn't on the menubar menu on Windows so I think we're good.

Comment 21

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #20)
> (In reply to Tim Nguyen [:ntim] from comment #19)
> > I think it's useful when clicking on File/Edit/View/...
> > But not on the menus themselves.
> 
> It isn't on the menubar menu on Windows so I think we're good.

Gijs pointed out that I was wrong about this. I happened to test this on a build with his patch still applied. This isn't a regression from Australis though so you can file a non-Australis bug (if there isn't one already) if you think it's worth changing.

Comment 22

[Tim Nguyen :ntim]

Should this be in Aurora ?

Comment 23

[:Gijs (he/him)]

Attachment: Patch for Aurora

Comment 24

[:Gijs (he/him)]

Comment on attachment 8372991 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Australis subview panels (for history, developer items, character encoding, feeds, etc.) will have a weird context menu
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd3253032193

Comment 26

[Ada [:adalucinet]]

Reproduced with Nightly from 2014-01-29.
Verified as fixed on Firefox 29 beta 4 (Build ID: 20140331125246) and latest Aurora (Build ID: 20140402004007): the toolbar context menu is no longer shown when right clicking in the dropdown list items from Web Developer Tools, Character Encoding, etc.
 
Verified on Mac OS X 10.9, Windows 7 x64, Windows 8.1 x64 and Ubuntu 12.04 x32:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0