1291835 - The last search engine can be removed using the Delete key

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Carlos [nohamelin]]

For the search pane of about:preferences, bug 1264475 lets to remove a search engine via keyboard besides the "Remove" button, but it lets remove all the search engines, something that it's not supported and it's not possible using the button.

Comment 1

[Florian Quèze [:florian]]

I guess we could add a test for document.getElementById("removeEngineButton").disabled before handling the key press.

Claas, is this something you would be interested in fixing?

Comment 2

[Claas Augner [:claas]]

Florian, thanks for the ping, I am interested and will provide a fix.

Comment 3

[Claas Augner [:claas]]

Attachment: Prevent last search engine from being removed

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8777930 [details] [diff] [review]
Prevent last search engine from being removed

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

Thanks for taking care of this!

::: browser/components/preferences/in-content/search.js
@@ +382,5 @@
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    if (this._engines.length == 1) {
> +      throw new Error("Cannot remove last engine!");

This seems like a nice sanity check to add, but I don't think pressing a keyboard shortcut should produce an error in the console (it's user error, not something broken in the software), so I think we should also add a check in onTreeKeyPress, eg. !document.getElementById("removeEngineButton").disabled

Comment 5

[Claas Augner [:claas]]

Attachment: Prevent last search engine from being removed

Comment 6

[Claas Augner [:claas]]

Thanks for the review.

To avoid coupling the keyboard behaviour to the button, I chose to extract a getter `EngineView#isLastRemainingEngineSelected`.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8777930 [details] [diff] [review]
Prevent last search engine from being removed

(in the future, please mark the previous version as obsolete when attaching a new one. Including a version number in the patch's description also helps)

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8778373 [details] [diff] [review]
Prevent last search engine from being removed

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

One trivial comment, looks good otherwise.

::: browser/components/preferences/in-content/search.js
@@ +489,5 @@
>    isCheckBox: function(index, column) {
>      return column.id == "engineShown";
>    },
>  
> +  get isLastRemainingEngineSelected() {

I would prefer this to be a function to be consistent with the isCheckBox immediately above.

Comment 9

[Claas Augner [:claas]]

Attachment: Prevent last search engine from being removed

Comment 10

[Claas Augner [:claas]]

Comment on attachment 8779560 [details] [diff] [review]
Prevent last search engine from being removed

I have changed `isLastRemainingEngineSelected` to be a function rather than a getter.

Note: I use `hg bzexport` to submit my patches. It seems as if it strips the bug number from the attachment description while leaving the description in the bug itself intact. If it's really necessary to have the bug number in the attachment description, the bzexport behavior should be changed. Also, I used `hg bzexport -e` with my second patch with does not allow to mark the previous patch as obsolete, while `hg bzexport -i` does, and I didn't verify that, but will in the feature.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8779560 [details] [diff] [review]
Prevent last search engine from being removed

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

Looks great except for a coding style detail I should have noticed before.

(In reply to Claas Augner [:claas] from comment #10)

> Note: I use `hg bzexport` to submit my patches. It seems as if it strips the
> bug number from the attachment description while leaving the description in
> the bug itself intact. If it's really necessary to have the bug number in
> the attachment description, the bzexport behavior should be changed.

The bug number in the attachment description wouldn't help. It's a patch version that should go there. Eg. "Patch v1", "Patch v2", etc.

> Also, I used `hg bzexport -e`

This lets you edit the description.

> with does not allow to mark the
> previous patch as obsolete, while `hg bzexport -i` does

Without option or with the -e option, hg bzexport will mark the previous version of the patch as obsolete if it has the same patch name. Your first attachment here had "1291835-last-search-engine" as the name, the second one had "bug-1291835-last-search-engine".

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

I'm sorry I missed this last time:
- we usually keep the operators at the end of the line.
- when mixing || and && in the same test, especially when things are slit on multiple lines, we put parentheses around the && parts, and the next line is indented with one more space.

Here the result would be:
      } else if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE ||
                 (isMac && aEvent.shiftKey &&
                  aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE &&
                  !gEngineView.isLastRemainingEngineSelected())) {

Comment 12

[Carlos [nohamelin]]

> +  isLastRemainingEngineSelected: function() {
> +    return this.selectedIndex == -1 || this.lastIndex == 0;
> +  },

By the way, the name of this function doesn't match really with its definition.

Comment 13

[Florian Quèze [:florian]]

(In reply to Carlos [nohamelin] from comment #12)
> > +  isLastRemainingEngineSelected: function() {
> > +    return this.selectedIndex == -1 || this.lastIndex == 0;
> > +  },
> 
> By the way, the name of this function doesn't match really with its
> definition.

Do you have a better suggestion? :-)

Comment 14

[Claas Augner [:claas]]

(In reply to Carlos [nohamelin] from comment #12)
> > +  isLastRemainingEngineSelected: function() {
> > +    return this.selectedIndex == -1 || this.lastIndex == 0;
> > +  },
> 
> By the way, the name of this function doesn't match really with its
> definition.

Thanks for pointing this out: it also returns true if no engine is selected at all. Therefore I suppose it'd be better to name it "isEngineSelectedAndRemovable" (inversing its return value):

> +  isEngineSelectedAndRemovable: function() {
> +    return this.selectedIndex != -1 && this.lastIndex != 0;
> +  },

Comment 15

[Claas Augner [:claas]]

Attachment: Prevent last search engine from being removed

If you're busy with your PTO, I'd appreciate if you could forward this to anybody else. Thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/095ec4971ffb
Prevent last search engine from being removed. r=florian

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/095ec4971ffb

Comment 18

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 51.0a1 (2016-08-03) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Aurora


Build ID            20161005004013
User Agent          Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

Comment 19

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Windows 7 x64 using Firefox 50.0.2. 

Verified fixed on Windows 7 x64, Mac OSX 10.12 and Ubuntu 14.04 x64 using Firefox 51 Beta 8 (buildID:  	20161215085501).