1064068 - Don't allow long-tap to initiate text-selection on non-text input elements

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

Long-tap actions on web-pages attempt to invoke element contextmenus. Failing that we try to invoke text-selection on underlying text.

Input elements (<input>, <textarea>) normally contain selectable text. However, buttons for example do not.

We currently try to select text on these elements and fail silently with a javascript error message such as:
[JavaScript Error: "TypeError: this._targetElement.QueryInterface(...).editor is null" {file: "chrome://browser/content/SelectionHandler.js" line: 744}]. I'd like to clean that up.

This is a simple fix to SelectionHandler.js, and requires basic knowledge of Javascript, and HTML. You should have an Android device, and be able to build the mobile version of Firefox to test your solution.

Comment 1

[Christian Vielma (IRC :cvielma)]

Hi! I'd like to give a try to this bug too. 

I'm a bit confused about the line where the error is. I found a line similar to the error in the _getEditor method on line 772 (http://hg.mozilla.org/mozilla-central/file/2255d7d187b2/mobile/android/chrome/content/SelectionHandler.js#l772). Is this the method the one that we need to prevent to be called on non-input elements?

Comment 2

[Mark Capella [:capella]]

To fix this, we need to modify the code at the beginning of SelectionHandlers longtap logic, to simply do an early (unsuccess) return.

At this point:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=070491691b4e&mark=307-307#292

We've completed any previous in-selection processing, and are about to start any new processing. We can simply add a check here to return "false" if the targeted / provided element for the operation is an input element and is *not* of type text.

TMI:
http://www.w3.org/wiki/HTML/Elements/input
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=070491691b4e&mark=800-800#798

Maybe above it create, and call from startSelection() a new method like "_isNonTextInputElement()" that returns true/false.

Testing this is easier. You can observe the problem before your patch by examining the android LOGCAT for the javascript error message. If your fix works, the error message goes away ( and no new ones appear ;-) ).

Comment 3

[Christian Vielma (IRC :cvielma)]

Thanks Mark! I'll start working on this and let you know if I need more information. It seems very straight forward, but getting used again to the tools and code may take me a bit ;)

Comment 4

[Christian Vielma (IRC :cvielma)]

Hey Mark, I'm submitting a patch for this, please let me know if it's ok. I've tested it and it's working :)

One question I have is about the input type. Should we only be checking type text or should we also check other types like email, tel, url and search (from: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-type)?

Comment 5

[Christian Vielma (IRC :cvielma)]

Attachment: Patch

Comment 6

[Mark Capella [:capella]]

Comment on attachment 8492693 [details] [diff] [review]
Patch

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

This is basically all I think we need, with some minor tweaks:

You mentioned you tested it? Great! Local LOGCAT examination I assume?

::: mobile/android/chrome/content/SelectionHandler.js
@@ +286,5 @@
>      }
>    },
>  
> +  _isNonTextInputElement: function(aElement) {
> +    if (aElement instanceof HTMLInputElement && aElement.type != 'text') {

Could you locate the method further down in the module? There's a similar method
isElementEditableText() we might group together.

Also let's be consistent with the way it checks for an element being "text",
using mozilla's |.mozIsTextField(false)|

Finally, you can abbreviate the method, just |return (foo && bar);| versus the more-verbose
|if (foo && bar) { return true; } else { return false; }|

Comment 7

[Christian Vielma (IRC :cvielma)]

Attachment: bug-1064068-fix.patch

New proposed patch update based on feedback.

Comment 8

[Christian Vielma (IRC :cvielma)]

(In reply to Mark Capella [:capella] from comment #6)
> Comment on attachment 8492693 [details] [diff] [review]
> Patch
> 
> Review of attachment 8492693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is basically all I think we need, with some minor tweaks:
> 
> You mentioned you tested it? Great! Local LOGCAT examination I assume?

That's correct. My test was with a simple form, trying to select input fields and other texts in the screen and trying to select the button. Before I was able to see the error log and after the fix is not shown.
> 
> ::: mobile/android/chrome/content/SelectionHandler.js
> @@ +286,5 @@
> >      }
> >    },
> >  
> > +  _isNonTextInputElement: function(aElement) {
> > +    if (aElement instanceof HTMLInputElement && aElement.type != 'text') {
> 
> Could you locate the method further down in the module? There's a similar
> method
> isElementEditableText() we might group together.
> 
> Also let's be consistent with the way it checks for an element being "text",
> using mozilla's |.mozIsTextField(false)|
I struggled a bit with this until I put it like in the patch (!aElement.mozIsTextField(false), because aElement.mozIsTextField(false) will still cause the error). 

Where can I find a definition of this(and potentially other) method? I saw that you mentioned in another bug that we can use mxr. I found that I can search for it here: http://lxr.mozilla.org/mozilla-central/search?string=mozIsTextField but it returns many results. 

> 
> Finally, you can abbreviate the method, just |return (foo && bar);| versus
> the more-verbose
> |if (foo && bar) { return true; } else { return false; }|

Thanks!

Comment 9

[Mark Capella [:capella]]

Comment on attachment 8492843 [details] [diff] [review]
bug-1064068-fix.patch

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

fyi - A more refined search for that method shows it:
http://mxr.mozilla.org/mozilla-central/ident?i=MozIsTextField

The actual method is implemented in c++ / xpcom here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#2358

A lot of this stuff you'll just pickup as you complete more patches and are exposed to other mentor / reviewers :)

Lastly, please update your commit message ... change the |r=capela| to |r=margaret| we'll ask her to do the final review
as I requested and mentored this :-)

::: mobile/android/chrome/content/SelectionHandler.js
@@ +762,5 @@
>              (aElement instanceof HTMLTextAreaElement));
>    },
>  
> +  _isNonTextInputElement: function(aElement) {
> +    return aElement instanceof HTMLInputElement && !aElement.mozIsTextField(false);

Can you wrap the return clause in parens? (Personal nit-style I prefer - not a major issue).
return (aElement instanceof HTMLInputElement && !aElement.mozIsTextField(false));

Comment 10

[Mark Capella [:capella]]

Quick fyi... when was the last time you updated your local repo from mozilla-central? You patch seems to be bit-rotted, and you'll have to rebase it.

Comment 11

[Christian Vielma (IRC :cvielma)]

Attachment: bug-1064068-fix.patch

Comment 12

[Christian Vielma (IRC :cvielma)]

Hi Mark, thanks for the information. I did the fix and added Margaret.

I'm using Aurora repo instead. I thought it was easier to test it in Aurora since it would be more stable. Still, I'm not that familiar with Mercurial so I did a hg pull -u and a hg pull --rebase (do I need to do anything else?) before submitting the latest one.

Let me know what's the best way to go :)

Comment 13

[Mark Capella [:capella]]

When your patch gets final review+, its migration path goes to fx-team, then on to mozilla-central.

fx-team is like an integration test environment where your patch is built with others on the way to m-c. Occasionally, your patch can conflict with changes existing there that haven't gotten to m-c yet, though that's somewhat rare.

Once a month m-c is merged to m-a.

You should be working from a version of m-c and try to keep it as close to that as possible, (pull once or more a week-ish). If you try to develop patches against m-a you will be working with base code that's 6 weeks or more behind, and most likely not apply when pushing to fx-team.

Comment 14

[Mark Capella [:capella]]

Slight correction:

Once a month m-c is merged to m-a.
-- should be --
Once per release cycle (around 6 weeks) m-c is merged to m-a.

fyi - https://wiki.mozilla.org/RapidRelease/Calendar

Comment 15

[:Margaret Leibovic]

Comment on attachment 8493497 [details] [diff] [review]
bug-1064068-fix.patch

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

Looks good! Thanks cvielma and capella!

I agree with capella, you should test this out on m-c to make sure it still applies and works as expected, since that's where it will land.

Comment 16

[Mark Capella [:capella]]

rebased for christian and push to TRY-server:
https://tbpl.mozilla.org/?tree=Try&rev=0b876dc68c96

Comment 17

[Mark Capella [:capella]]

try push just fine, pushing to fx-team (and we're done!)
https://hg.mozilla.org/integration/fx-team/rev/82fb13581cf6

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82fb13581cf6