790898 - Match desktop in restrictions for "Add Search Engine" option

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Firefox 18.0a1 (2012-09-12)
Device: Galaxy Note
OS: Android 4.0.4

Steps to reproduce:
1. Open Firefox for Android
2. Go to about:feedback and tap on I have an idea
3. Long tap on the text field

Expected result:
Add Search Engine is not displayed in the context options.

Actual result:
After step 3, when context menu is triggered, Add Search Engine option is listed as you can see in the attached screenshot. Nothing happens if it is tapped.

Comment 1

[Cristian Nicolae (:xti)]

Attachment: screenshot

Comment 3

[Chenxia Liu [:liuche]]

Desktop also supports adding search engines for most form fields, but has more restrictions than Android.

We should at least match Desktop in the checks we do.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1486

Comment 4

[Chenxia Liu [:liuche]]

The desktop link is bitrotted, but the checks are in "isTargetAKeywordField", and the relevant Android code is in mobile/android/chrome/content/browser.js, at the "filter" function in SearchEngines.init() [1].

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6642

Comment 5

[Chenxia Liu [:liuche]]

Wes or Brian, or whomever - I think I might have a mentee who's interested in working on this bug, but since I'll be on away for 2 weeks, can you keep an eye on this and help them out if they need it? Thanks!

Comment 6

[raymond [:retornam] (inactive)]

Attachment: bug-790898.patch

Comment 7

[:Margaret Leibovic]

Comment on attachment 8341199 [details] [diff] [review]
bug-790898.patch

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

Thanks for taking this on! This is a good start, but it looks like we're still missing some checks that desktop does. Here's a better link to that code:
http://hg.mozilla.org/mozilla-central/file/4bf430d990e5/browser/base/content/nsContextMenu.js#l1503

::: mobile/android/chrome/content/browser.js
@@ +6642,5 @@
>      let filter = {
>        matches: function (aElement) {
> +        if (!(aElement instanceof HTMLInputElement))
> +          return false;
> +        else {

You don't need an else statement after a return.

@@ +6643,5 @@
>        matches: function (aElement) {
> +        if (!(aElement instanceof HTMLInputElement))
> +          return false;
> +        else {
> +          return (aElement.form && NativeWindow.contextmenus.textContext.matches(aElement));

This NativeWindow.contextmenus check is somewhat redundant with the `instanceof HTMLInputElement` check above. I think we should just go ahead and replace the body of this `matches` function with the body of desktop's `isTargetAKeywordField` function (and add a comment saying that's where we got that logic from).

Comment 8

[raymond [:retornam] (inactive)]

Attachment: bug-790898.patch

Comment 9

[raymond [:retornam] (inactive)]

Attachment: bug-790898.patch

Comment 10

[:Margaret Leibovic]

Comment on attachment 8342541 [details] [diff] [review]
bug-790898.patch

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

Nice, this looks good. I just have a few small issues to address before we land this.

::: mobile/android/chrome/content/browser.js
@@ +6629,5 @@
>  
>      let filter = {
>        matches: function (aElement) {
> +        //Copied over from body of isTargetAKeywordField 
> +        //function in nsContextMenu.js

Nit: This could all be one line, and you should add a space after the // before the text (e.g. // Copied...)

@@ +6632,5 @@
> +        //Copied over from body of isTargetAKeywordField 
> +        //function in nsContextMenu.js
> +        if(!(aElement instanceof HTMLInputElement))
> +          return false;
> +        var  form = aElement.form;

Nit: Looks like you accidentally put 2 spaces after var. And please include an empty line above this line.

We can also update this code to use `let` instead of `var` (the code in nsContextMenu.js was written before `let` existed).

Comment 11

[raymond [:retornam] (inactive)]

Attachment: bug-790898.patch

Comment 12

[:Margaret Leibovic]

Thanks for the patch!

https://hg.mozilla.org/integration/fx-team/rev/a7f8d2324426

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7f8d2324426