Allow removal of search engines using "Delete" key.

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Preferences
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: claas, Assigned: claas)

Tracking

Trunk
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox48 affected, firefox50 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Firefox Desktop's preferences tab "Search" (about:preferences#search) includes a manageable list of "One-click search engines". A search engine can be removed by first selecting the engine's row and by then either clicking the "Remove" button or alternatively pressing the keyboard shortcut |ALT+SHIFT+R|.

I suggest to also allow the removal of search engines using the "Delete" key and will provide the corresponding patch to add this functionality.
(Assignee)

Comment 1

2 years ago
Created attachment 8741153 [details] [diff] [review]
search.js (Remove engine upon "Delete" keypress.)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8741153 - Flags: review?(dolske)
Comment on attachment 8741153 [details] [diff] [review]
search.js (Remove engine upon "Delete" keypress.)

I'm going to bounce this review to Florian.

I'd note that this breaks using Delete as a keyboard shortcut for "navigate back 1 page" (which is what currently happens), but that's probably ok when an item in the list is focused.
Attachment #8741153 - Flags: review?(dolske) → review?(florian)
(Assignee)

Comment 3

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #2)
> I'd note that this breaks using Delete as a keyboard shortcut for "navigate
> back 1 page" (which is what currently happens), but that's probably ok when
> an item in the list is focused.

Thanks for pointing that out. How can I trigger this "navigate back 1 page" shortcut you mention? Currently for me, the "Delete" key does not trigger any action at all when an item in the search engine list is focused.
Flags: needinfo?(dolske)
On Windows it's backspace that goes back.
Comment on attachment 8741153 [details] [diff] [review]
search.js (Remove engine upon "Delete" keypress.)

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

Thanks for the patch and sorry for the delayed review.

This doesn't work correctly on Mac. When I try the patch, the behavior I get is that the selected engine is removed, and the "back" action is triggered too, meaning that the search pane of the preference window is replaced by whatever pane I was looking at before (typically the General pane). To fix this you would need to add an eEvent.preventDefault() call.

That said, I'm not sure breaking the default backspace behavior of going back is a good idea.

How would you feel about using the delete key everywhere (that's fn+backspace on a macbook keyboard), and maybe also shift+backspace (that works on other lists, eg. the completions in the location bar)? I don't think we need any mac-specific behavior here.

::: browser/components/preferences/in-content/search.js
@@ +221,3 @@
>          tree.startEditing(index, tree.columns.getLastColumn());
> +      } else if (isMac && aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE ||
> +                !isMac && aEvent.keyCode == KeyEvent.DOM_VK_DELETE) {

nit: there's one space of indent missing on this line, the '!' should be aligned with the 'isMac' of the line above.
Attachment #8741153 - Flags: review?(florian) → review-
(I think mak answered the question for me.)
Flags: needinfo?(dolske)
(Assignee)

Comment 7

a year ago
Created attachment 8756938 [details] [diff] [review]
Allow removal of search engines using "Delete" key
(Assignee)

Comment 8

a year ago
Comment on attachment 8756938 [details] [diff] [review]
Allow removal of search engines using "Delete" key

I followed Florian's suggestion and removed the mac-specific behaviour to avoid breaking the default backspace behaviour.
Attachment #8756938 - Flags: review?(florian)
(In reply to Claas Augner [:claas] from comment #8)
> Comment on attachment 8756938 [details] [diff] [review]
> Allow removal of search engines using "Delete" key
> 
> I followed Florian's suggestion and removed the mac-specific behaviour to
> avoid breaking the default backspace behaviour.

Did you not like the suggestion of also using shift+backspace?
Comment on attachment 8756938 [details] [diff] [review]
Allow removal of search engines using "Delete" key

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

The current patch works and I see no issue with it, so r=me, but I think I would have made it use shift+backspace too if I was the one who wrote it ;-).
Attachment #8756938 - Flags: review?(florian) → review+
(Assignee)

Comment 11

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> but I think I would have made it use shift+backspace too

Sorry for the delay and thanks for your review and the clarification. In fact, I assumed that Mac possibly maps shift+backspace or fn+backspace to delete, but as I understand it now this is not the case.

Actually, I don't have any particular objection against adding these mac-specific shortcuts (shift+backspace and fn+backspace), but overall I would definitely opt for consistency.

Having said this, there aren't so many places that use backspace [0]. Among these, however, more use DOM_VK_DELETE and DOM_VK_BACK_SPACE as is [1a-e] and fewer take into consideration shiftKey [2a-b] or metaKey. On the other hand, as long as there aren't any UX guidelines for keyboard shortcuts, we might as well decide to the best of our knowledge.

In your opinion, Florian, does it make sense to restrict the shortcuts shift+backspace and fn+backspace to Mac and/or to restrict delete to Non-Mac? At least shift+backspace doesn't work for me in the location bar on Linux.

[0] https://dxr.mozilla.org/mozilla-central/search?q=DOM_VK_BACK_SPACE+-path%3Atest&redirect=false
[1a] https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/cookies.js#747-753
[1b] https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/permissions.js#352-357
[1c] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.js#284-289
[1d] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManagerExceptions.js#94-99
[1e] https://dxr.mozilla.org/mozilla-central/source/toolkit/profile/content/profileSelection.js#137-142
[2a] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#943-951
[2b] https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#506-515
Flags: needinfo?(florian)
(In reply to Claas Augner [:claas] from comment #11)
> (In reply to Florian Quèze [:florian] [:flo] from comment #10)
> > but I think I would have made it use shift+backspace too
> 
> Sorry for the delay and thanks for your review and the clarification. In
> fact, I assumed that Mac possibly maps shift+backspace or fn+backspace to
> delete, but as I understand it now this is not the case.

fn+backspace is mapped to delete; the patch works when typing fn+backspace on a macbook keyboard as you support DOM_VK_DELETE.

> Actually, I don't have any particular objection against adding these
> mac-specific shortcuts (shift+backspace and fn+backspace), but overall I
> would definitely opt for consistency.
> 
> Having said this, there aren't so many places that use backspace [0]. Among
> these, however, more use DOM_VK_DELETE and DOM_VK_BACK_SPACE as is [1a-e]
> and fewer take into consideration shiftKey [2a-b] or metaKey. On the other
> hand, as long as there aren't any UX guidelines for keyboard shortcuts, we
> might as well decide to the best of our knowledge.

It seems to me that on Mac backspace works directly in popup windows, and requires pressing the shift key for UI elements that can be seen in the main browser window, where backspace is already mapped to moving back to the previous page.

> In your opinion, Florian, does it make sense to restrict the shortcuts
> shift+backspace and fn+backspace to Mac and/or to restrict delete to
> Non-Mac? At least shift+backspace doesn't work for me in the location bar on
> Linux.

Forget fn+backspace, it's the same thing as delete.
After looking at the dxr links you gave (thanks for researching this!), I think it would make sense to use shift+backspace on mac only.
Flags: needinfo?(florian)
(Assignee)

Comment 13

a year ago
Created attachment 8762760 [details] [diff] [review]
Allow removal of search engines using keyboard shortcuts
Attachment #8762760 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8756938 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8741153 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8762761 [details] [diff] [review]
Allow removal of search engines using keyboard shortcuts
Attachment #8762761 - Flags: review?(florian)
(Assignee)

Updated

a year ago
Attachment #8762760 - Attachment is obsolete: true
Attachment #8762760 - Flags: review?(florian)
Comment on attachment 8762761 [details] [diff] [review]
Allow removal of search engines using keyboard shortcuts

Looks great to me, thanks for the update patch! :-)
Attachment #8762761 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/1db14ffd2e94e2ab314a429772ee59edee287185
Bug 1264475 - Allow removal of search engines using keyboard shortcuts. r=florian

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1db14ffd2e94
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I have reproduced this on Firefox nightly according to(2016-04-13)

It's verified on Latest Nightly--Build ID:( 20160715063552 ), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0

now this is allow to use 'Delete' keyboard shortcut for removing search engines.

Tested OS--Windows10 32bit
QA Whiteboard: [bugday-20160713]
The Bug was about to Allow removal of search engines using "Delete" key. I have seen the implementation on Latest Nightly 50.0a1 on Ubuntu 16.04, 64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 50.0a1

Build ID: 20160727030230
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Updated

a year ago
Depends on: 1291835
You need to log in before you can comment on or make changes to this bug.