Text selection does not select on handle crossover in <inputs> and <textarea>; handles dissapear

RESOLVED FIXED in Firefox 29



Firefox for Android
Text Selection
4 years ago
4 years ago


(Reporter: aaronmt, Assigned: maxli)


({reproducible, testcase})

Firefox 29
reproducible, testcase

Firefox Tracking Flags



(Whiteboard: [parity-chrome][parity-android][mentor=margaret][lang=js], URL)


(1 attachment, 1 obsolete attachment)

3.53 KB, patch
: review+
Details | Diff | Splinter Review


4 years ago
Currently our text-selection functionality will not invert a selection (create a new selection) on handle crossover for <input> and <textarea>. As of right now, we handle the case appropriately for raw text.

See video: http://youtu.be/0OH4hReoMGg

We should duplicate what is expected for <input> and <textarea>.

Nightly (01/15)
LG Nexus 4 (Android 4.4.2)


4 years ago
Keywords: reproducible, testcase

Comment 1

4 years ago
FYI, not a recent regression:

It looks like this was intentional fallout from our work to get rid of the mouse event hacks, but I guess we failed to file a follow-up for it :/
Blocks: 667243
tracking-fennec: ? → +

Comment 2

4 years ago
I can mentor this, but I don't have an answer for how to write a patch. Fixing this will require investigating the APIs we can use for selection in editable elements, since they're different than the ones we use for non-editable elements.
Whiteboard: [parity-chrome][parity-android] → [parity-chrome][parity-android][mentor=margaret][lang=js]


4 years ago
QA Contact: aaron.train

Comment 3

4 years ago
Created attachment 8365779 [details] [diff] [review]
Attachment #8365779 - Flags: review?(margaret.leibovic)

Comment 4

4 years ago
Comment on attachment 8365779 [details] [diff] [review]

Review of attachment 8365779 [details] [diff] [review]:

Thanks for writing a patch for this bug! This looks like a good approach, but I'd just like to keep the boolean logic we have around left/right handles all in one place.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +534,5 @@
> +   * @param aX the handle's x-coordinate in client coordinates
> +   * @param aCaretPos the current position of the caret
> +   */
> +  _moveSelectionInEditable: function sh_moveSelectionInEditable(aIsLeftHandle, aX, aCaretPos) {
> +    let anchorX = aIsLeftHandle ^ this._isRTL ? this._cache.end.x

Any reason why you chose to use aIsLeftHandle instead of aIsStartHandle? It seems confusing to have these two different concepts in the file. Also, you're repeating some of the conditional logic from _moveSelection in here.

I think it would be clearer to just pass in a value for anchorX to this function, so that it doesn't need to know about which handle is being moved.

You can also refactor _moveSelection if you can think of a cleaner way to organize that logic.
Attachment #8365779 - Flags: review?(margaret.leibovic) → feedback+


4 years ago
Assignee: nobody → maxli

Comment 5

4 years ago
Created attachment 8366476 [details] [diff] [review]
Patch v2

(In reply to :Margaret Leibovic from comment #4)
> Any reason why you chose to use aIsLeftHandle instead of aIsStartHandle?

I removed it now, but the reason I had it there originally is because the start handle differs between being on the left and on the right depending on the text direction (i.e. RTL or LTR), so knowing whether the start handle is being moved and the coordinates of the two handles isn't enough to know if they've crossed over.
Attachment #8365779 - Attachment is obsolete: true
Attachment #8366476 - Flags: review?(margaret.leibovic)

Comment 6

4 years ago
Comment on attachment 8366476 [details] [diff] [review]
Patch v2

Review of attachment 8366476 [details] [diff] [review]:

Thanks for the explanation and thanks for addressing my comment, this logic can be tricky :/

::: mobile/android/chrome/content/SelectionHandler.js
@@ +531,5 @@
> +   * Helper function for moving the selection inside an editable element.
> +   *
> +   * @param aAnchorX the stationary handle's x-coordinate in client coordinates
> +   * @param aX the moved handle's x-coordinate in client coordinates
> +   * @param aCaretPos the current position of the caret

Nice documentation.
Attachment #8366476 - Flags: review?(margaret.leibovic) → review+


4 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [parity-chrome][parity-android][mentor=margaret][lang=js] → [parity-chrome][parity-android][mentor=margaret][lang=js][fixed-in-fx-team]
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [parity-chrome][parity-android][mentor=margaret][lang=js][fixed-in-fx-team] → [parity-chrome][parity-android][mentor=margaret][lang=js]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.