870471 - Contextual menu in Australis Customization toolbar and menu panel

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

Add contextual menu based on the mock-up.

Comment 1

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: Mock-up

Comment 2

[:Gijs (he/him)]

Attachment: Patch

This should essentially work and match the spec, I believe.

I needed to pull the contextmenu into the panel to make the selected mode not flicker on hover the time (see eg. bug 492960 comment 20 for another report of this from Paolo).

I added contextmenu event listeners to avoid the (disabled) contextmenu firing when you right click while already in customize mode.

I added ellipsis for entering customize mode, and made the string for the panel's context menu say "Add more items..." (note the s).

This implementation has one known bug: the normal context menu in the toolbar stops working if you move the button to the panel and back (until you restart, and only for that window). This is a core issue which I've filed as bug 877669. I'll try and come up with a fix for that, which I think shouldn't be toooo hard. Alternatively, we can workaround in two different ways: we can manually assign the same contextmenu item for things in the existing toolbox as exists on the toolbar; or we could not use the attributes and do manual contextmenu event listening + opening everywhere. I don't much like the idea of the latter option, I have to say, and I'd say the impact here is minimal and the chances of a proper fix high, hence me doing nothing so far.

Finally, I might add that these context menus are normal context menus and therefore don't match the look and feel as shown on the mockup, but rather match the ordinary platform context menus. I assume that won't be a problem.

Comment 3

[:Gijs (he/him)]

Ah, one more thing: I guess I haven't tested this patch with an overflowing navbar. UX-wise, I guess we should show the button before the overflow (possibly nudging another existing item into the overflow) if people add a button to the toolbar. Does the overflowable toolbar expose an API to let us do that?

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 756006 [details] [diff] [review]
Patch

>+    <menupopup id="customizationContextMenu">
>+      <menuitem oncommand="gCustomizeMode.addToToolbar(document.popupNode)"
>+                label="&customizeMenu.addToToolbar.label;"/>
>+      <menuitem oncommand="gCustomizeMode.removeFromPanel(document.popupNode)"
>+                label="&customizeMenu.removeFromMenu.label;"/>
>+      <menuseparator/>
>+      <menuitem command="cmd_CustomizeToolbars"
>+                label="&viewCustomizeToolbar.label;"/>
>+    </menupopup>
>+
>+    <menupopup id="customizationPanelContextMenu">
>+      <menuitem command="cmd_CustomizeToolbars"
>+                label="&customizeMenu.addMoreItems.label;"/>
>+    </menupopup>

Lacks access keys.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 756006 [details] [diff] [review]
> Patch
> 
> >+    <menupopup id="customizationContextMenu">
> >+      <menuitem oncommand="gCustomizeMode.addToToolbar(document.popupNode)"
> >+                label="&customizeMenu.addToToolbar.label;"/>
> >+      <menuitem oncommand="gCustomizeMode.removeFromPanel(document.popupNode)"
> >+                label="&customizeMenu.removeFromMenu.label;"/>
> >+      <menuseparator/>
> >+      <menuitem command="cmd_CustomizeToolbars"
> >+                label="&viewCustomizeToolbar.label;"/>
> >+    </menupopup>
> >+
> >+    <menupopup id="customizationPanelContextMenu">
> >+      <menuitem command="cmd_CustomizeToolbars"
> >+                label="&customizeMenu.addMoreItems.label;"/>
> >+    </menupopup>
> 
> Lacks access keys.

Yes, because the menubutton isn't keyboard accessible in the first place, as far as I can tell...

Comment 6

[Dão Gottwald [:dao]]

That doesn't really matter for context menu. For instance, you can't open the toolbar context menu for toggling toolbars with the keyboard, but the items in the menu are still keyboard accessible.

Comment 7

[:Gijs (he/him)]

Attachment: Patch v1.1

This patch adds accesskeys. :-)

Comment 8

[:Gijs (he/him)]

Attachment: Patch v1.2

So it looks like in bug 877669 we won't change the behaviour of context="" or contextmenu="" because we have existing code (and possibly add-ons) that depend on that working. Removing the attribute outright ought to work, though.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 756470 [details] [diff] [review]
Patch v1.2

>+<!ENTITY customizeMenu.addToToolbar.label "Add to Toolbar">
>+<!ENTITY customizeMenu.addToToolbar.accesskey "A">
>+<!ENTITY customizeMenu.removeFromMenu.label "Remove from Menu">
>+<!ENTITY customizeMenu.removeFromMenu.accesskey "R">
>+<!ENTITY customizeMenu.addMoreItems.label "Add more itemsâ¦">

Should be "Add More Items" (mind the caps)

I'm not familiar with the CustomizableUI.jsm code, so that's best reviewed by somebody else.

Comment 10

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

Just a note that this patch is likely bitrotted by bug 873867.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 756470 [details] [diff] [review]
Patch v1.2

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

What comment 9 said.

Comment 12

[:Gijs (he/him)]

Pushed, https://hg.mozilla.org/projects/ux/rev/9da5648f063b

Comment 13

[Martin Giger [:freaktechnik]]

I am using a windows 32 bit UX 24.0a1 (2013-06-06) build on Windows 7 64-bit, with the latest patch before build being 788d4dc9aaca.
The context menu works, but for some buttons it consistently closes the panel on right click. The buttons affected are Preferences, Add-ons and Private Browsing. I haven't tested all available buttons thought.

Comment 14

[uy2251+bugzilla]

What happens when addons that have their own custom right-click menus are placed in the panel? Is this menu overridden?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to uy2251+bugzilla from comment #14)
> What happens when addons that have their own custom right-click menus are
> placed in the panel? Is this menu overridden?

We *should* append these new menuitems to the bottom of the custom context menu that the add-on provided. Can you please file a new bug for this?

Comment 16

[uy2251+bugzilla]

Would it not make more sense to have the add-on menuitems first? Surely in that case the add-on menuitems would be the primary interaction and customization a secondary interaction?

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to uy2251+bugzilla from comment #16)
> Would it not make more sense to have the add-on menuitems first? Surely in
> that case the add-on menuitems would be the primary interaction and
> customization a secondary interaction?

Yes, that is what I meant :)

Comment 18

[uy2251+bugzilla]

Just re-read your comment and realized I need to read these things properly. I've never filed a bug report before. How would I go about it?

Comment 19

[:Gijs (he/him)]

(In reply to uy2251+bugzilla from comment #18)
> Just re-read your comment and realized I need to read these things properly.
> I've never filed a bug report before. How would I go about it?

Go here: https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox&component=Toolbars%20and%20Customization

And fill in something sensible for the "Summary" and "Description" fields that matches what you think the bug should be about. There's a field called "Blocks" (bottom-ish right). Please add the bug number of this bug in there. For bonus points, put [Australis:M?] in the whiteboard field. Thank you very much for helping us improve the UX builds! If you have any issues, no worries, one of us can do it too, just let us know. :-)

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/9da5648f063b