Closed Bug 960203 Opened 10 years ago Closed 10 years ago

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

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(fennec+)

RESOLVED FIXED
Firefox 29
Tracking Status
fennec + ---

People

(Reporter: aaronmt, Assigned: maxli)

References

()

Details

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

Attachments

(1 file, 1 obsolete file)

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)
FYI, not a recent regression:
http://hg.mozilla.org/mozilla-central/annotate/09a8ed7cbb0b/mobile/android/chrome/content/SelectionHandler.js#l583

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: ? → +
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]
QA Contact: aaron.train
Attached patch Patch (obsolete) — Splinter Review
Attachment #8365779 - Flags: review?(margaret.leibovic)
Comment on attachment 8365779 [details] [diff] [review]
Patch

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+
Assignee: nobody → maxli
Attached patch Patch v2Splinter Review
(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 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+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/afed99fd2c9f
Keywords: checkin-needed
Whiteboard: [parity-chrome][parity-android][mentor=margaret][lang=js] → [parity-chrome][parity-android][mentor=margaret][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/afed99fd2c9f
Status: NEW → RESOLVED
Closed: 10 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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: