Closed Bug 927911 Opened 6 years ago Closed 6 years ago

Don't try to attach SelectionHandler carets to disabled input elements

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file)

Attached patch bugtextselSplinter Review
Caught this while working with bug 840144 ... without protection we get an error in SelectionHandler.js

Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIQueryContentEventResult.left]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/SelectionHandler.js :: sh_getHandlePositions :: line 615"  data: no]" {file: "chrome://browser/content/browser.js" line: 4293}]

That's currently here:
Attachment #818507 - Flags: review?(margaret.leibovic)
Comment on attachment 818507 [details] [diff] [review]
bugtextsel

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

Nice catch. r+ with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +4300,5 @@
>              this._sendMouseEvent("mousedown", element, x, y);
>              this._sendMouseEvent("mouseup",   element, x, y);
>  
> +            // See if its a input element, that isn't disabled
> +            if (!element.getAttribute("disabled")) {

We should just check !element.disabled here, rather than using getAttribute. We can also combine this into the existing if statement, like:

if (!element.disabled &&
    ((element instanceof HTMLInputElement && element.mozIsTextField(false)) ||
     (element instanceof HTMLTextAreaElement)))

That's starting to get pretty gnarly, but I think it's still okay.
Attachment #818507 - Flags: review?(margaret.leibovic) → review+
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Before I push it, can you explain this part?
((element instanceof HTMLInputElement && element.mozIsTextField(false))

Why do we prevent the caret from appearing in an input field containing text?
(In reply to Mark Capella [:capella] from comment #2)
> Before I push it, can you explain this part?
> ((element instanceof HTMLInputElement && element.mozIsTextField(false))
> 
> Why do we prevent the caret from appearing in an input field containing text?

I'm confused by your question. How does this prevent the caret from appearing in an input field containing text? Isn't it just checking to make sure the input *is* a text field?

Looking at blame, this was added back in bug 652168. Looking through the comments there, I see we just added this to match the input context menu selector:
https://bugzilla.mozilla.org/show_bug.cgi?id=652168#c21
Ok, I was wrong. There's another input field we're using for new about:config that wasn't getting a caret as it was excluded here but ... turns out the input is defined not as "text" but as "numeric".

I may have to allow for that also, either here or in a later bug
s/numeric/number/
https://hg.mozilla.org/mozilla-central/rev/9c0174ed4276
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.