Closed Bug 1064068 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: capella, Assigned: cvielma, Mentored)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 file, 2 obsolete files)

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.
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?
Flags: needinfo?(markcapella)
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 ;-) ).
Flags: needinfo?(markcapella)
Assignee: nobody → cvielma
Status: NEW → ASSIGNED
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 ;)
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)?
Attached patch Patch (obsolete) — Splinter Review
Attachment #8492693 - Flags: review?(markcapella)
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; }|
Attachment #8492693 - Flags: review?(markcapella) → feedback+
Attached patch bug-1064068-fix.patch (obsolete) — Splinter Review
New proposed patch update based on feedback.
Attachment #8492843 - Flags: review?(markcapella)
(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 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));
Attachment #8492843 - Flags: review?(markcapella) → feedback+
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.
Attachment #8492693 - Attachment is obsolete: true
Attachment #8492843 - Attachment is obsolete: true
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 :)
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.
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
Attachment #8493497 - Flags: review?(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.
Attachment #8493497 - Flags: review?(margaret.leibovic) → review+
rebased for christian and push to TRY-server:
https://tbpl.mozilla.org/?tree=Try&rev=0b876dc68c96
try push just fine, pushing to fx-team (and we're done!)
https://hg.mozilla.org/integration/fx-team/rev/82fb13581cf6
https://hg.mozilla.org/mozilla-central/rev/82fb13581cf6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.