Pressing Delete key in Extension Manager should uninstall extension

VERIFIED FIXED in mozilla1.9beta3

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: Simon Bünzli)

Tracking

({polish})

Trunk
mozilla1.9beta3
polish
Points:
---
Bug Flags:
blocking1.9 -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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

13 years ago
Created attachment 200905 [details] [diff] [review]
Proposed Patch

Updated

13 years ago
Attachment #200905 - Flags: review+

Updated

13 years ago
Attachment #200905 - Flags: review+

Updated

13 years ago
Attachment #200905 - Flags: review?(robert.bugzilla)
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.
Attachment #200905 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 4

11 years ago
Created attachment 266310 [details] [diff] [review]
[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.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #266310 - Flags: review?(robert.bugzilla)

Comment 5

11 years ago
Nom'ing for the download manager portion as well.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3
Not a blocker.
Flags: blocking-firefox3? → blocking-firefox3-
Keywords: polish
Attachment #266310 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Simon, this has bitrotted in downloads.js. Can you post an updated version then I'll check it in
(Assignee)

Comment 8

11 years ago
Created attachment 271628 [details] [diff] [review]
unbitrotted patch for check-in

Thanks, Dave.
Attachment #266310 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 3 → Firefox 3 M7
The Download manager part maybe caused Bug 387695

Updated

11 years ago
Depends on: 387695

Comment 11

11 years ago
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 ?
(Assignee)

Comment 12

11 years ago
(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.
Flags: in-litmus?

Comment 13

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

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.
Flags: in-litmus? → in-litmus-
(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...
Flags: in-litmus- → in-litmus?
(or reopen this bug if the fix never actually worked)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

10 years ago
Created attachment 291901 [details] [diff] [review]
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.
Attachment #291901 - Flags: review?(dtownsend)
(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+
Flags: in-litmus? → in-litmus+
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).
Attachment #291901 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 21

10 years ago
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).
Attachment #291901 - Flags: review- → review?(dtownsend)

Updated

10 years ago
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
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".
Attachment #291901 - Flags: review?(dtownsend) → review+
(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.

(Assignee)

Comment 24

10 years ago
Created attachment 292837 [details] [diff] [review]
follow-up fix (for check-in)
Attachment #291901 - Attachment is obsolete: true
Attachment #292837 - Flags: approval1.9?

Updated

10 years ago
Attachment #292837 - Flags: approval1.9? → approval1.9+
(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
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 3 M7 → Firefox 3 M11
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.