303194 - Pressing Delete key in Extension Manager should uninstall extension

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Jesse Ruderman]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802
Firefox/1.0+

Steps to reproduce:
1. Open Extension Manager.
2. Press the Delete key.

Result: Nothing happens.

Expected: Show an uninstall warning dialog, as if I had pressed the Uninstall
button.

Comment 1

[Mel Reyes]

I don't have the code pulled down, but I just hacked something together with Local Install, but the code would look something like this, right?

Add:
<key id="key_uninstall"  keycode="VK_DELETE" command="cmd_uninstall" oncommand="gExtensionsViewController.doCommand('cmd_uninstall');"/>

Update this entry:
<menuitem id="menuitem_uninstall" command="cmd_uninstall" key="key_uninstall"
label="&cmd.uninstall.label;" accesskey="&cmd.uninstall.accesskey;"/>

Comment 2

[Dave Reid]

Attachment: Proposed Patch

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 200905 [details] [diff] [review]
Proposed Patch

Though keybinding discovery is a good thing doing so in the context menu is not optimal even though that is the only place to do so at present... perhaps when the ui is next re-written a better solution for keybinding discovery will be included.

r=me without adding the key to the menuitem.

btw: I think it would also be appropriate to add similar functionality to the download manager for the consistency.

Comment 4

[Simon Bünzli]

Attachment: [Del] for extensions and downloads

Ignoring the fact that the Add-ons manager has a tendency of eating key presses (due to the commandupdater not really working) this patch now works for both add-ons and downloads in pretty much the same way.

Comment 5

[Basil Hashem [:baz]]

Nom'ing for the download manager portion as well.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker.

Comment 7

[Dave Townsend [:mossop]]

Simon, this has bitrotted in downloads.js. Can you post an updated version then I'll check it in

Comment 8

[Simon Bünzli]

Attachment: unbitrotted patch for check-in

Thanks, Dave.

Comment 9

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.66; previous revision: 1.65
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.57; previous revision: 1.56
done

Comment 10

[Carsten Book [:Tomcat]]

The Download manager part maybe caused Bug 387695

Comment 11

[Przemyslaw Bialik]

I've tested a few trunk builds since the patch was checked in (today tested on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072304 Minefield/3.0a7pre) and I can't verify this bug as fixed.
Pressing delete key does completely nothing in addons/extensions manager, only ALT+U shortcut shows uninstall confirmation dialog.
For testing I've replaced
<key id="key_uninstall" keycode="VK_DELETE"                             command="cmd_uninstall"
         oncommand="gExtensionsViewController.doCommand('cmd_uninstall');"/>
with
<key id="key_uninstall" keycode="VK_DELETE"                             command="cmd_close"
         oncommand="gExtensionsViewController.doCommand('cmd_uninstall');"/>
and after this delete key closed EM as expected.
Any ideas, reopening ?

Comment 12

[Simon Bünzli]

(In reply to comment #11)
That's a different bug (see comment #4), though I'm not sure if it's ever been filed as such.

Comment 13

[Przemyslaw Bialik]

(In reply to comment #12)
Could you please file this bug because I think you would describe it better ?

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

I've not yet gotten this to work, so I'm not going to write a Litmus testcase for a feature I can't reliably use.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #14)
> I've not yet gotten this to work, so I'm not going to write a Litmus testcase
> for a feature I can't reliably use.

Shouldn't you instead file a new bug and leave this in-litmus? until it's fixed? Or at the very least, leave this in-litmus? so that someone else who can use the feature can write a test...

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(or reopen this bug if the fix never actually worked)

Comment 17

[Dave Townsend [:mossop]]

I think I ever tested the result of this patch and just checked it in. Just checked on the nightly from after the landing and I can't get the delete key to do anything so I conclude that the bug as it stands was not fixed.

Comment 18

[Simon Bünzli]

Attachment: remove hindering command attribute

So, copying the line above didn't work out as that one was buggy as well... Removing the command attributes where an oncommand attribute exists does the trick.

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #15)
> (In reply to comment #14)
> > I've not yet gotten this to work, so I'm not going to write a Litmus testcase
> > for a feature I can't reliably use.
> 
> Shouldn't you instead file a new bug and leave this in-litmus? until it's
> fixed? Or at the very least, leave this in-litmus? so that someone else who can
> use the feature can write a test...

Fair enough; I didn't know whom that would be, since I (wrongly) assumed that comment 4 meant this fix was "as good as it got," due to the add-ons manager eating keypresses.  I now see my error, and have written:

https://litmus.mozilla.org/show_test.cgi?id=5020

in-litmus+

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 291901 [details] [diff] [review]
remove hindering command attribute

Sorry I meant to get to this sooner.

This is essentially right however on OSX you need to also make the backspace key uninstall items since this is used more than delete (but both should work there).

Comment 21

[Simon Bünzli]

Comment on attachment 291901 [details] [diff] [review]
remove hindering command attribute

This is just a follow-up to the above patch which already got in (including Litmus-test), so I won't get fancy here. Please file a new bug for your enhancement request (which then BTW should apply to the Downloads Manager as well).

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 291901 [details] [diff] [review]
remove hindering command attribute

I think that it's easier to just take the change here rather than a whole new bug given that it's such a small addition. It should only be adding a new key element ifdeffed appropriately. I also really wouldn't like to consider this bug fixed without the change.

r=me with that addition.

Stephen, it's probably worth updating the litmus testcase to be a little less ambiguous for the OSX case. The general delete key for items like this is just backspace, in fact on the macbooks there is no key labelled "delete".

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #22)
> 
> Stephen, it's probably worth updating the litmus testcase to be a little less
> ambiguous for the OSX case. The general delete key for items like this is just
> backspace, in fact on the macbooks there is no key labelled "delete".

There is on my MacBook Pro, but you're right: I've updated the Litmus testcase to specifically call out Backspace/Delete as both functioning the same.

Comment 24

[Simon Bünzli]

Attachment: follow-up fix (for check-in)

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #21)
> (From update of attachment 291901 [details] [diff] [review])
> This is just a follow-up to the above patch which already got in (including
> Litmus-test), so I won't get fancy here. Please file a new bug for your
> enhancement request (which then BTW should apply to the Downloads Manager as
> well).

@ Simon: Download Manager has had this for some time already -> bug 394457

@ mossop: Sorry, I now realize you meant the older macbooks

Comment 26

[Dave Townsend [:mossop]]

Thanks Simon!

Checking in toolkit/mozapps/extensions/content/extensions.xul;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.xul,v  <--  extensions.xul
new revision: 1.67; previous revision: 1.66
done

Stephen, I think Mac are just a little inconsistent about their key labelling. My MacBook is pretty new but has no "Delete". I'm told that MacBook Pros of the same time have a "delete" and the keyboard on the Mac Mini doesn't.

Comment 27

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121805 Minefield/3.0b3pre. I also verified on Intel Mac using the same nightly. Verified using the testcase in Comment 19.