1264475 - Allow removal of search engines using "Delete" key.

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Claas Augner [:claas]]

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.

Comment 1

[Claas Augner [:claas]]

Attachment: search.js (Remove engine upon "Delete" keypress.)

Comment 2

[Justin Dolske [: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.

Comment 3

[Claas Augner [:claas]]

(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.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

On Windows it's backspace that goes back.

Comment 5

[Florian Quèze [:florian]]

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.

Comment 6

[Justin Dolske [:Dolske]]

(I think mak answered the question for me.)

Comment 7

[Claas Augner [:claas]]

Attachment: Allow removal of search engines using "Delete" key

Comment 8

[Claas Augner [:claas]]

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.

Comment 9

[Florian Quèze [: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 10

[Florian Quèze [:florian]]

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 ;-).

Comment 11

[Claas Augner [:claas]]

(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

Comment 12

[Florian Quèze [: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.

Comment 13

[Claas Augner [:claas]]

Attachment: Allow removal of search engines using keyboard shortcuts

Comment 14

[Claas Augner [:claas]]

Attachment: Allow removal of search engines using keyboard shortcuts

Comment 15

[Florian Quèze [: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! :-)

Comment 16

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/1db14ffd2e94e2ab314a429772ee59edee287185
Bug 1264475 - Allow removal of search engines using keyboard shortcuts. r=florian

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1db14ffd2e94

Comment 18

[Saheda Reza [:Antora]]

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

Comment 19

[Farhadur Reja Fahim]

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