880164 - Australis toolbar buttons contextual menu in toolbar, palette and customize mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

Bug 870471 has implemented a contextual menu for the panel menu items and the panel itself, the buttons on the toolbar should also have a contextual basically containing this :

[Add to menu]
[Remove from toolbar]
------------------
[Customize]

Comment 1

[:Gijs (he/him)]

The toolbars already have a contextual menu with a "Customize" entry, plus toggles for the toolbars. I don't know that we want to change that. UX?

Comment 2

[u428464]

I think it would be more consistent. I've tested latest UX builds and it feels great to be able to add the menu item to the toolbar without using the customization window, but it feels painful to have to open customization mode to do the same the other way.

Comment 3

[u428464]

Maybe this would be better when you right click a toolbar button :

[Menu Bar]
[Bookmarks Toolbar]
------------------
[Add to Menu]
[Remove from Toolbar]
------------------
[Customize]

Comment 4

[Alejandro Rodriguez [:Alopepeo]]

In case you right click on an addon toolbar button, would it be posible to let the user disable, uninstall, configure or enter the addons manager directly though the context menu?

Thanks.

Comment 5

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

(In reply to :Gijs Kruitbosch from comment #1)
(In reply to Guillaume C. [:ge3k0s] from comment #3)

I think we should add "Remove from Toolbar" when user right click on a widgets. Not so much to allow customization without entering the customization page, but to provide a quick way to remove undesirable add-ons when we remove the add-ons bar and move all the add-ons into the toolbar.

(In reply to Alejandro Rodriguez [:Alopepeo] from comment #4)
I don't think jamming all the management into the contextual menu is a good idea, but I do think we should add ability to open the "Add-ons Manager" when right click on an add-on in the toolbar.

Comment 6

[u428464]

FWIW the Bwinton's live mockup ( http://people.mozilla.com/~bwinton/australis/customization/mac/ ) showed the "Add to menu" item and it was very nice to use.

Comment 7

[Matthew N. [:MattN]]

1) I think we should also include the context menu for buttons in the palette since drag and drop is hard for some users.

2) For consistency, I think the context menu should work in the panel menu while in customization mode

Comment 8

[Justin Dolske [:Dolske]]

In addition to a basic Remove command, it would be good to allow moving to the menu panel (or navbar, if already in the menu panel). In fact I see that's what we've already done for the menu panel, items there have entries for "Add to Toolbar", "Remove from Menu, and "Customize…".

Comment 10

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

We should also add context menu for buttons in customization mode.

For buttons in the "more tools" palette show "Add to toolbar" & "Add to menu"
For buttons in Menu panel show "Move to toolbar" "Remove"
For buttons in Toolbar show "Move to Menu Panel" "Remove"

Comment 12

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

Attachment: WIP patch

checkpointing my work here.

Comment 13

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

Attachment: WIP patch v2

Further along, still need to get the context menu working on the panel and palette items when in customization mode.

Comment 14

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

Attachment: WIP patch v3

Added a test for this. Need to figure out why the context menu isn't showing up for the panel or the palette when in customization mode.

Comment 15

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

Attachment: Patch

I fixed the remaining issues with the test not passing.

1:09.44 INFO TEST-START | Shutdown
1:09.44 Browser Chrome Test Summary
1:09.44        Passed: 1042
1:09.44        Failed: 0
1:09.44        Todo: 0

Comment 16

[:Gijs (he/him)]

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

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

f+ because this mostly seems to work, but there are some significant issues that I've highlighted below.

Further to the specific code comments, I noticed two things:
1) When context-clicking the star in the bookmarks button (didn't try the menu part of it), the add/remove options are greyed out (!?);
2) Context-clicking the home button brings up no context menu at all.

::: browser/base/content/browser.js
@@ +4161,5 @@
> +        break;
> +      toolbarItem = parent;
> +    }
> +  }
> +  let movable = toolbarItem && toolbarItem.getAttribute &&

If toolbarItem is non-null, how would it not have a getAttribute method?

::: browser/base/content/browser.xul
@@ +278,2 @@
>        <menuseparator/>
> +      <menuseparator id="viewToolbarsMenuSeparator"/>

Why the extra separator with an id? I don't see this being used elsewhere...

::: browser/components/customizableui/content/panelUI.inc.xul
@@ -137,5 @@
> -      <menuitem command="cmd_CustomizeToolbars"
> -                accesskey="&customizeMenu.addMoreItems.accesskey;"
> -                label="&customizeMenu.addMoreItems.label;"/>
> -    </menupopup>
> -  </popupset>

This was intentionally inside the panel. See bug 870471 comment 2, bug 492960 comment 20, etc. With the patch, on OS X retina, I see flickering again when moving through the context menu. Please keep these menus here. AFAICT it shouldn't really affect the rest of the code. Feel free to add a comment above it to indicate why this is here.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +476,5 @@
> +    const kCustomizationPaletteItemContextMenu = "customizationPaletteItemContextMenu";
> +    let contextMenuMap = {
> +      "panel": kCustomizationPanelItemContextMenu,
> +      "toolbar": kCustomizationToolbarItemContextMenu,
> +      "palette": kCustomizationPaletteItemContextMenu

I don't think this code should care about the palette, and with the code you've added in customize mode, I think stuff would work even if you removed this one. In which case it might be easier to just have an if/else statement or ternary expression rather than a full on place-to-context-menu-map.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +488,5 @@
>        wrapper.setAttribute("flex", aNode.getAttribute("flex"));
>      }
>  
> +    if (aNode.hasAttribute("context")) {
> +      wrapper.setAttribute("context", aNode.getAttribute("context"));

This will forward the node's original context attribute even in the case of an add-on/button-provided context menu. I don't think we want that - the wrapper should always have our context menu in customize mode. The wrapper should probably set it the same way the ensureContextMenu code in CustomizableUI does it, and maybe even remove a context/contextmenu attribute on aNode itself if present, and store it temporarily and bring it back when unwrapping, etc. like we do for the other attributes.

@@ -736,5 @@
>          break;
> -      case "contextmenu":
> -        aEvent.preventDefault();
> -        aEvent.stopPropagation();
> -        break;

Right, so now add-on-provided menus and so on will also work. We need to be smarter here. See my other suggestion about (re)storing these attributes for customize mode.

Note also that this means right-clicking on an empty space in the panel in customization mode shows a single-menu "Add more Items" item that is disabled (because you're already in customize mode). I think we should just disable this contextmenu entirely when in customize mode (not the ones on items, but we *should* disable the panel-wide one).

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +10,5 @@
> +
> +function test() {
> +  waitForExplicitFinish();
> +  testNotCustomizingPart1();
> +}

Is there a particular reason not to use the mini-test-framework in head.js for this test? IMO yield and promises would clean up some of the callback-based stuff in this test.

@@ +20,5 @@
> +    expectedEntries = [
> +      [".customize-context-addToPanel", true],
> +      [".customize-context-removeFromToolbar", true],
> +      ["---"],
> +      ["#toggle_toolbar-menubar", true],

This will fail on mac because there's no menubar to toggle. Ditto further down.

Comment 17

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

Attachment: Patch v1.1

All feedback should be addressed. Pushed to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=8c272a2c8902

Comment 18

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

Comment on attachment 8334794 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/browser.xul
@@ +278,2 @@
>        <menuseparator/>
> +      <menuseparator id="viewToolbarsMenuSeparator"/>

You asked about this in the previous review. This id is referenced 10 lines earlier in the onpopupshowing attribute, and is needed to insert the toolbar menuitems in the middle of the contextmenu.

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +8,5 @@
>         level="top"
>         hidden="true"
>         noautofocus="true">
>    <panelmultiview id="PanelUI-multiView" mainViewId="PanelUI-mainView">
> +    <panelview id="PanelUI-mainView" context="customizationPanelContextMenu">

I switched from contextmenu to context because context appears to be the preferred attribute name and MDN says that contextmenu is an alias for context.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +514,5 @@
> +    if (aPlace != "toolbar") {
> +      wrapper.setAttribute("context", contextMenuForPlace);
> +    }
> +    if (currentContextMenu) {
> +      // todo, should maintain whether it was using "contextmenu" or "context" for add-on compat.

Sorry, I missed this, but it would be trivial to fix this todo after your review.

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +27,5 @@
> +}
> +
> +let gTests = [
> +  {
> +    desc: "",

I forgot to put descriptions in for these tests, but I can do that before checking in/in next version.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8334794 [details] [diff] [review]
Patch v1.1

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

Sorry, but I'm going to give you another f+ because of the range offset uncertainty and the undefined variable.

::: browser/base/content/browser.js
@@ +4154,5 @@
> +  // If the right-click was on anonymous content crossing a scope, then the
> +  // explicitOriginalTarget will point at the context menu. Need to do extra
> +  // work to find the element that was actually clicked on.
> +  if (toolbarItem.localName == "menupopup") {
> +    // Not sure why we need to subtract 1 from rangeOffset here.

What's rangeOffset? Can you provide more detail here? This comment doesn't really inspire confidence. :-(

OOI, what happens if you right click SDK widgets with an iframe in them? I'm OK dealing with those in a followup, as this is enough pain as-is, but it'd be good to know.

@@ +4173,5 @@
> +  }
> +
> +  let movable = toolbarItem && toolbarItem.getAttribute("removable") == "true";
> +  popup.querySelector(".customize-context-addToPanel").disabled = !movable;
> +  popup.querySelector(".customize-context-removeFromToolbar").disabled = !movable;

Have you checked that these setters work all the time, even on e.g. first-time show of the context menu, when the XBL bindings might not have been defined yet? (not sure about this one)

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +487,3 @@
>        if (!currentContextMenu)
> +        aNode.setAttribute("context", contextMenuForPlace);
> +    } else if (currentContextMenu in contextMenus) {

Where is this defined? I don't see it anywhere. That's not great...

@@ +1934,5 @@
>  
>      return true;
> +  },
> +
> +  getPlaceForItem: function(aElement) {

Please add this directly onto CustomizableUI. You're even calling the CustomizableUI version from the other code in here so there's no reason for the extra level of indirection.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +510,5 @@
> +                             aNode.getAttribute("contextmenu");
> +    let contextMenuForPlace = aPlace == "panel" ?
> +                                kPanelItemContextMenu :
> +                                kPaletteItemContextMenu;
> +    if (aPlace != "toolbar") {

Hmm? If we're making this work in customize mode, why not make it work everywhere? That way this'll work even for stuff like the urlbar/search text fields.

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +40,5 @@
> +      let expectedEntries = [
> +        [".customize-context-addToPanel", true],
> +        [".customize-context-removeFromToolbar", true],
> +        ["---"],
> +        ["#toggle_toolbar-menubar", true],

This should be conditional here, too.

@@ +69,5 @@
> +      let expectedEntries = [
> +        [".customize-context-addToPanel", true],
> +        [".customize-context-removeFromToolbar", true],
> +        ["---"],
> +        ["#toggle_toolbar-menubar", true],

And here

@@ +128,5 @@
> +      let shownContextPromise = contextMenuShown(contextMenu);
> +      let newWindowButton = document.getElementById("new-window-button");
> +      ok(newWindowButton, "new-window-button was found");
> +      EventUtils.synthesizeMouse(newWindowButton, 2, 2, {type: "contextmenu", button: 2});
> +      yield shownContextPromise;

Can you check here that the panel hasn't been hidden yet?

@@ +229,5 @@
> +  {
> +    desc: "",
> +    setup: null,
> +    run: function () {
> +    },

Uhh, please remove these

@@ +255,5 @@
> +}
> +
> +// This is a simpler version of the context menu check that
> +// exists in contextmenu_common.js.
> +function checkContextMenu(aContextMenu, aExpectedEntries) {

Please add the global fns at the top or the bottom, not both.

Comment 20

[BuTaMuH]

Would it be possible the Customize Button also to be moveable ?

Comment 21

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

(In reply to :Gijs Kruitbosch from comment #19)
> ::: browser/base/content/browser.js
> @@ +4154,5 @@
> > +  // If the right-click was on anonymous content crossing a scope, then the
> > +  // explicitOriginalTarget will point at the context menu. Need to do extra
> > +  // work to find the element that was actually clicked on.
> > +  if (toolbarItem.localName == "menupopup") {
> > +    // Not sure why we need to subtract 1 from rangeOffset here.
> 
> What's rangeOffset? Can you provide more detail here? This comment doesn't
> really inspire confidence. :-(

I can't find any documentation on rangeOffset. It appears to be a Mozilla extension for this event.
 
> OOI, what happens if you right click SDK widgets with an iframe in them? I'm
> OK dealing with those in a followup, as this is enough pain as-is, but it'd
> be good to know.

I added a test for this locally. We don't get the event if the user right-clicks on the Widget-provided image.

I'm going to limit the scope of this bug so that it doesn't try to fix this shortcoming in the Add-on SDK or deal with anonymous content scope changes.

> @@ +4173,5 @@
> > +  }
> > +
> > +  let movable = toolbarItem && toolbarItem.getAttribute("removable") == "true";
> > +  popup.querySelector(".customize-context-addToPanel").disabled = !movable;
> > +  popup.querySelector(".customize-context-removeFromToolbar").disabled = !movable;
> 
> Have you checked that these setters work all the time, even on e.g.
> first-time show of the context menu, when the XBL bindings might not have
> been defined yet? (not sure about this one)

I haven't seen any issues with this and when I run the test it runs through this code before the context menu is otherwise shown, but I can add a null-check on the result of these querySelector calls.

Comment 22

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #21)
> > @@ +4173,5 @@
> > > +  }
> > > +
> > > +  let movable = toolbarItem && toolbarItem.getAttribute("removable") == "true";
> > > +  popup.querySelector(".customize-context-addToPanel").disabled = !movable;
> > > +  popup.querySelector(".customize-context-removeFromToolbar").disabled = !movable;
> > 
> > Have you checked that these setters work all the time, even on e.g.
> > first-time show of the context menu, when the XBL bindings might not have
> > been defined yet? (not sure about this one)
> 
> I haven't seen any issues with this and when I run the test it runs through
> this code before the context menu is otherwise shown, but I can add a
> null-check on the result of these querySelector calls.

That's not what I mean, I meant that the .disabled property is in XBL, and so if the binding hasn't been constructed setting the property will break the property from then on. We dealt with this earlier with the customize/help button on session restore, remember that one? Might be safer to just set/remove the attributes.

Comment 23

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

Attachment: Patch v1.2

Fixed the issues with the previous version of the patch. Removed the rangeOffset code. I noticed a bug with using these context menuitems when in customization mode and I'll file a follow-up bug for it.

Comment 24

[:Gijs (he/him)]

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

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

Looks good, so you get an r+, but you should look at the comment I made in browser.xul. I think that explains the issue with the addWidget/removeWidget stuff you were seeing, and you should probably fix it before landing as it's specific to these changes. Should be possible to just grab the firstChild of the wrapper if it's localName == "toolbarpaletteitem". Please nullcheck, though, because we're already seeing too many issues with add-ons breaking some of our assumptions.

Given the test, might be worth considering a try run.

::: browser/base/content/browser.xul
@@ +270,5 @@
> +      <menuitem oncommand="gCustomizeMode.addToPanel(document.popupNode)"
> +                accesskey="&customizeMenu.addToPanel.accesskey;"
> +                label="&customizeMenu.addToPanel.label;"
> +                class="customize-context-addToPanel"/>
> +      <menuitem oncommand="gCustomizeMode.removeFromArea(document.popupNode)"

Actually, I suspect this might be the problem of the wrapping stuff. In customize mode, you add the context attribute to the toolbarpaletteitems. That means those will be the document.popupNodes. The methods in CustomizeMode.jsm should check for this possibility and move the wrapped widget ID, not the original one.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1939,5 @@
>        }
>      }
>  
>      return true;
> +  },

Please leave this. :-)

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ +46,5 @@
> +      let shownPromise = contextMenuShown(contextMenu);
> +      let urlBarContainer = document.getElementById("urlbar-container");
> +      // Need to make sure not to click within an edit field.
> +      let urlbarRect = urlBarContainer.getBoundingClientRect();
> +      EventUtils.synthesizeMouse(urlBarContainer, 100, urlbarRect.height - 1, {type: "contextmenu", button: 2 });

This is highly magical... have you checked this on 3 platforms? Might be worth doing a try run...

Comment 25

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

Try run, https://tbpl.mozilla.org/?tree=Try&rev=63aa234df64f

Comment 26

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

https://hg.mozilla.org/integration/fx-team/rev/4b21f6a03667

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b21f6a03667

Comment 28

[Francesco Lodolo [:flod]]

Why the same accesskey? Are these items mutually exclusive?

<!ENTITY customizeMenu.removeFromToolbar.label "Remove from Toolbar">
<!ENTITY customizeMenu.removeFromToolbar.accesskey "R">
<!ENTITY customizeMenu.removeFromMenu.label "Remove from Menu">
<!ENTITY customizeMenu.removeFromMenu.accesskey "R">

Comment 29

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #28)
> Why the same accesskey? Are these items mutually exclusive?
> 
> <!ENTITY customizeMenu.removeFromToolbar.label "Remove from Toolbar">
> <!ENTITY customizeMenu.removeFromToolbar.accesskey "R">
> <!ENTITY customizeMenu.removeFromMenu.label "Remove from Menu">
> <!ENTITY customizeMenu.removeFromMenu.accesskey "R">

Yes.