Closed Bug 958869 Opened 6 years ago Closed 6 years ago

Stop showing toolbar context menu on Australis widgets panels

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: ntim, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 1 obsolete file)

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.
Whiteboard: [Australis:P1]
I don't understand why this should be P1. P4, maybe, and even that seems on the high side...
Whiteboard: [Australis:P1] → [Australis:P4]
I don't understand what the issue is here. It was a decision to implement a contextual menu for widgets.
(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?
Flags: needinfo?(ntim007)
I've just tested it and it seems to be the issue indeed.
(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.
Flags: needinfo?(ntim007)
(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?
Flags: needinfo?(ntim007)
(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).
Flags: needinfo?(ntim007)
1. left click on the developer widget
2. right click on the developer subview
actual results: nothings happens
What am I missing here ?
Flags: needinfo?(ntim007)
(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.
Attached image panel-contextmenu.png
Flags: needinfo?(ntim007)
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
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.
(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)
Summary: Stop showing toolbar context menu on Australis widgets panels → Stop showing toolbar context menu on Australis widgets panels and on menus
Whiteboard: [Australis:P4] → [Australis:P3]
This should do the trick.
Attachment #8372705 - Flags: review?(MattN+bmo)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8372705 - Flags: review?(MattN+bmo) → review-
(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
Summary: Stop showing toolbar context menu on Australis widgets panels and on menus → Stop showing toolbar context menu on Australis widgets panels
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b07709c0ea92
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
(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.
(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.
(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.
Depends on: 969966
Should this be in Aurora ?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch Patch for AuroraSplinter Review
Attachment #8372705 - Attachment is obsolete: true
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
Attachment #8372991 - Flags: approval-mozilla-aurora?
Attachment #8372991 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.