Disable "Add to Firefox" button in right-click menu after installing extension

VERIFIED FIXED in mozilla1.9.1b1

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: tchung, Assigned: Régis Caspar)

Tracking

unspecified
mozilla1.9.1b1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008061104 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008061104 Minefield/3.0pre

Need to disable the "add to firefox..." option in the right-click context menu after completing an addon installation.  Otherwise, the user can keep installing the addon before restarting.

See screenshot.

Reproducible: Always

Steps to Reproduce:
1. launch trunk
2. open addons manager > get addons, and search for an extension
3. install the extension
4. when status says "Install complete", right click the extension area in the same window.
5. Verify the option to "Add to firefox" is still enabled
Actual Results:  
"Add to firefox" in right-click context menu is still enabled

Expected Results:  
disable right-click menu after installing
(Reporter)

Comment 1

10 years ago
Created attachment 324716 [details]
add to firefox screenshot
Product: Firefox → Toolkit

Updated

10 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 2

10 years ago
Created attachment 335367 [details] [diff] [review]
patch

Patch hiding the "Add to Firefox..." entry from the context menu when the selected add-on has been installed (and disable the associated command).
Additionally, this fix some issues regarding the context menu:
- JavaScript error: chrome://mozapps/content/extensions/extensions.js, line 1708: menuitem_about is null
- JavaScript error: chrome://mozapps/content/extensions/extensions.js, line 1715: document.getElementById("menuitem_cancelUninstall_clone") is null

Tested on a Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080822071548 Minefield/3.1a2pre ID:20080822071548 custom build
Assignee: nobody → regis.caspar+bz
Status: NEW → ASSIGNED
Attachment #335367 - Flags: review?(dtownsend)
Comment on attachment 335367 [details] [diff] [review]
patch

This is good, just a couple of minor issues to clear up.

> 
>-  // All views support about
>-  var menuitem_about = document.getElementById("menuitem_about_clone");
>-  var name = selectedItem ? selectedItem.getAttribute("name") : "";
>-  menuitem_about.setAttribute("label", getExtensionString("aboutAddon", [name]));
>-
>+  // All views (but search) support about 
>+  if (gView != "search") {
>+    var menuitem_about = document.getElementById("menuitem_about_clone");
>+    var name = selectedItem ? selectedItem.getAttribute("name") : "";
>+    menuitem_about.setAttribute("label", getExtensionString("aboutAddon", [name]));
>+  }
>+  
>   /* When an update or install is pending allow canceling the update or install

While it isn't causing a problem right now, the plugins view doesn't use the about menuitem so may as well avoid the extra work for that case too.

>     switch (aCommand) {
>     case "cmd_installSearchResult":
>-      return true;
>+      return selectedItem.getAttribute("action") != "installed";
>     case "cmd_useTheme":

There are more states here than just installed. If you start the install then right click before the confirmation message or before the install is complete this still shows the menu as enabled. Probably want to disable unless there is no action or the action is "failed"

If you felt like it you might also look at bug 415579 while you are working through this.
Attachment #335367 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 4

10 years ago
Created attachment 335511 [details] [diff] [review]
patch v2

Updated patch taking into account Dave's remarks. This fix bug 415579, bug 419956 and this bug. 
Does this need UI-review because of the added button and context-menu changes?
Attachment #335367 - Attachment is obsolete: true
Attachment #335511 - Flags: review?(dtownsend)
Comment on attachment 335511 [details] [diff] [review]
patch v2

Yeah this will need a ui-review, please take a screenshot and request one from jboriss before asking for review on a new patch. We might end up just not taking the button on the installs pane and just using making the option available in the context menu I guess.

This is pretty good work, I hadn't expected to see the cancel install option on the search pane but that's good thinking. Just a few mostly minor issues...

> // For Firefox don't display context menuitems that can open a browser window.
> var gUpdateContextMenusNoBrowser = ["menuitem_installUpdate", "menuitem_includeUpdate"];
>-var gInstallContextMenus = ["menuitem_homepage", "menuitem_about"];
>-var gSearchContextMenus = ["menuitem_learnMore", "menuitem_installSearchResult"];
>+var gInstallContextMenus = ["menuitem_homepage", "menuitem_about", "menuitem_cancelInstall"];

I think we should add a separator in between the first 2 items and the cancel items. You also need to include the cancelUpgrade menu item. Neither of those should be necessary on the search pane though.

>   /* When an update or install is pending allow canceling the update or install
>      and don't allow uninstall. When an uninstall is pending allow canceling the
>      uninstall.*/
>-  if (gView != "updates" && gView != "installs") {
>+  if (gView != "updates" && gView != "installs" && gView != "search") {
>     var canEnable = gExtensionsViewController.isCommandEnabled("cmd_cancelUninstall");
>     document.getElementById("menuitem_cancelUninstall_clone").hidden = !canEnable;
>     var canCancelInstall = gExtensionsViewController.isCommandEnabled("cmd_cancelInstall");
>     document.getElementById("menuitem_cancelInstall_clone").hidden = !canCancelInstall;
>     var canCancelUpgrade = gExtensionsViewController.isCommandEnabled("cmd_cancelUpgrade");
>     document.getElementById("menuitem_cancelUpgrade_clone").hidden = !canCancelUpgrade;
>     document.getElementById("menuitem_uninstall_clone").hidden = canEnable || canCancelInstall || canCancelUpgrade;
>   }

The installs and search panes do actually do some of the bits in this block, I can't really tell without trying but might be possible to split this up to simplify this function a bit, see what you think.

>   case "installs":
>+    /* Allow uninstall from the install view context menu. */
>+    var canCancelInstall = gExtensionsViewController.isCommandEnabled("cmd_cancelInstall");
>+    document.getElementById("menuitem_cancelInstall_clone").hidden = !canCancelInstall;
>+    break;
>+  case "search":
>+    var canInstall = gExtensionsViewController.isCommandEnabled("cmd_installSearchResult");
>+    document.getElementById("menuitem_installSearchResult_clone").hidden = !canInstall;
>+    document.getElementById("menuitem_cancelInstall_clone").hidden = canInstall;

Should be checking if cmd_cancelInstall is enabled or not for the cancelInstall menuitem. I was seeing weirdness where if you start the install then switch away and back to the search pane then the cancel install option was enabled even before the install had completed.

>   document.getElementById("locales-view").hidden = !showLocales;
>   document.getElementById("updates-view").hidden = !showUpdates;
>   document.getElementById("installs-view").hidden = !showInstalls;
>+  
>+  // fall back to another view since "installs" became hidden
>+  if (!showInstalls && gView == "installs")
>+    showView("search");
> }

This feels a little weird. When does this happen?

>     switch (aCommand) {
>     case "cmd_installSearchResult":
>-      return true;
>+      return (selectedItem.getAttribute("action") == "" || selectedItem.getAttribute("action") == "failed") ;

Style nit, we valiantly try to keep lines under 80 characters, so line break as appropriate. Also shouldn't need the brackets around the entire expression.

>     case "cmd_cancelUninstall":
>       return selectedItem.opType == OP_NEEDS_UNINSTALL;
>     case "cmd_cancelInstall":
>-      return selectedItem.opType == OP_NEEDS_INSTALL;
>+      return (selectedItem.getAttribute("action") != "" && 
>+            selectedItem.getAttribute("action") != "failed") ||
>+            selectedItem.opType == OP_NEEDS_INSTALL;

We can only cancel an install that is completed, so just the "installed" action I think. I think I'd prefer to verify that the search pane is visible before trusting the action attribute too. Another style nit, avoid the surrounding brackets and line up the start of lines (so the selectedItem's would all be in line here).

>     cmd_cancelInstall: function (aSelectedItem)
>     {
>       var name = aSelectedItem.getAttribute("name");
>       var result = false;
>+
>+      // to handle uninstall from the search view we trick aSelectedItem.opType
>+      if (aSelectedItem.getAttribute("action") != "" && 
>+          aSelectedItem.getAttribute("action") != "failed" && 
>+          gView == "search") {
>+        aSelectedItem.opType = OP_NEEDS_INSTALL;
>+      }
>+

As above, we only cancel something that is already installed. Rather than tampering with aSelectedItem instead just pull opType into a local variable, either from the action or the actual opType. Check the currently displayed pane to know which.

>+        <xul:hbox flex="1" pack="end">
>+          <xul:button class="installShow cancelInstallButton" label="&cancelInstall.label;"
>+                        accesskey="&cancelInstall.accesskey;" tooltiptext="&cmd.cancelInstall.tooltip;"
>+                        command="cmd_cancelInstall"/>

Line up the attributes at the start of the line please.
Attachment #335511 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 6

10 years ago
Created attachment 335686 [details]
Screenshot showing the new button in the install pane

(In reply to comment #5)
> (From update of attachment 335511 [details] [diff] [review])
> Yeah this will need a ui-review, please take a screenshot and request one from
> jboriss before asking for review on a new patch. We might end up just not
> taking the button on the installs pane and just using making the option
> available in the context menu I guess.
Ok I will ask him.

> >+  // fall back to another view since "installs" became hidden
> >+  if (!showInstalls && gView == "installs")
> >+    showView("search");
> > }
> 
> This feels a little weird. When does this happen?
This happens when you cancel the install on the "install" pane and there's no more items in this pane. The pane gets hidden by UpdateOptionalView (not sure about the name) and the user ends with no pane selected.

I will look into the other issues later. Thanks.
Attachment #335686 - Flags: ui-review?
(Assignee)

Updated

10 years ago
Attachment #335686 - Flags: ui-review? → ui-review?(jboriss)
This looks good, let's move forward with it and review on the patch.  I was surprised to see a cancel for the add-on install but agree that it's a good idea.
(Assignee)

Comment 8

10 years ago
Created attachment 335923 [details] [diff] [review]
patch v3

Updated following Dave comments. 
I just changed the way we go back to a view when "install" is hidden, now we go back to the previously selected view.
Attachment #335511 - Attachment is obsolete: true
Attachment #335923 - Flags: ui-review?(jboriss)
Attachment #335923 - Flags: review?(dtownsend)

Updated

10 years ago
Attachment #335923 - Flags: ui-review?(jboriss)
Attachment #335923 - Flags: ui-review+
Attachment #335923 - Flags: review?(dtownsend)
Attachment #335923 - Flags: review+
Comment on attachment 335923 [details] [diff] [review]
patch v3

This is great work, just a couple of minor nits and then it's ready for checkin.

>   /* When an update or install is pending allow canceling the update or install
>      and don't allow uninstall. When an uninstall is pending allow canceling the
>      uninstall.*/
>-  if (gView != "updates" && gView != "installs") {
>-    var canEnable = gExtensionsViewController.isCommandEnabled("cmd_cancelUninstall");
>-    document.getElementById("menuitem_cancelUninstall_clone").hidden = !canEnable;
>-    var canCancelInstall = gExtensionsViewController.isCommandEnabled("cmd_cancelInstall");
>+  if (gView != "updates") {
>     document.getElementById("menuitem_cancelInstall_clone").hidden = !canCancelInstall;
>-    var canCancelUpgrade = gExtensionsViewController.isCommandEnabled("cmd_cancelUpgrade");
>-    document.getElementById("menuitem_cancelUpgrade_clone").hidden = !canCancelUpgrade;
>-    document.getElementById("menuitem_uninstall_clone").hidden = canEnable || canCancelInstall || canCancelUpgrade;
>+
>+    if (gView != "installs" && gView != "search")
>+      document.getElementById("menuitem_cancelUninstall_clone").hidden = !canCancelUninstall;
>+
>+    if (gView != "search")
>+      document.getElementById("menuitem_cancelUpgrade_clone").hidden = !canCancelUpgrade;
>+
>+    if (gView != "installs" && gView != "search")
>+      document.getElementById("menuitem_uninstall_clone").hidden = canCancelUninstall || 
>+                                                                   canCancelInstall || 
>+                                                                   canCancelUpgrade;

Compress the uninstall and cancelUninstall bits into one if block.

>+  // fall back to the previously selected view or "search" since "installs" became hidden
>+  if (!showInstalls && gView == "installs")
>+  {
>+    var viewGroup = document.getElementById("viewGroup");
>+    var lastSelectedView = "search";
>+
>+    if (viewGroup.hasAttribute("last-selected"))
>+      lastSelectedView = viewGroup.getAttribute("last-selected");
>+
>+    showView(lastSelectedView);
>+  }

Nit, in our style rules the opening brace goes on the same line as the if.

>+      var opType = aSelectedItem.opType;
>+
>+      // to handle uninstall from the search view we trick opType
>+      if (aSelectedItem.getAttribute("action") == "installed" && gView == "search")
>+        opType = OP_NEEDS_INSTALL;

Just change the comment to read something like "A search result with an action "installed" is equivalent to a pending install"
Comment on attachment 335686 [details]
Screenshot showing the new button in the install pane

ui-r+ per comment 7
Attachment #335686 - Flags: ui-review?(jboriss) → ui-review+
(Assignee)

Comment 11

10 years ago
Created attachment 336340 [details] [diff] [review]
patch v4

Updated version following comment #10
Attachment #335923 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Blocks: 419956
(Assignee)

Updated

10 years ago
Blocks: 415579
Landed: http://hg.mozilla.org/mozilla-central/rev/eb481b7dfdb1

If you set checkin-needed on a bug then it reminds me to actually do something with it :)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]

Updated

10 years ago
Target Milestone: --- → mozilla1.9.1b1

Comment 13

10 years ago
The new vbox/hbox/vbox structure of an extension richlistitem is overly complex.
Instead of:
  +-----+----------------------------+
  |Icon | name, version              |
  |Icon | description                |
  +-----+----------------------------+
  |                [button] [button] |
  +----------------------------------+
One could also do:
  +-----+----------------------------+
  |Icon | name, version              |
  |Icon | description                |
  |     |          [button] [button] |
  +----------------------------------+
Which requires much less boxes.
You're welcome to provide a patch, however I don't see how it alters the number of boxes. In fact with it as it is now we could probably do away with the hbox surrounding the buttons but not with the alternate way you suggest. This is also only for the install pane, not for the add-ons in general.
(Assignee)

Comment 15

10 years ago
(In reply to comment #12)
> Landed: http://hg.mozilla.org/mozilla-central/rev/eb481b7dfdb1
Thanks
> If you set checkin-needed on a bug then it reminds me to actually do something
> with it :)
OK
(Assignee)

Comment 16

10 years ago
Verified Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080905031348 Minefield/3.1b1pre ID:20080905031348
Status: RESOLVED → VERIFIED

Updated

10 years ago
Duplicate of this bug: 419862
You need to log in before you can comment on or make changes to this bug.