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)
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)
3.53 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•10 years ago
|
Keywords: reproducible,
testcase
Comment 1•10 years ago
|
||
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
Updated•10 years ago
|
tracking-fennec: ? → +
Comment 2•10 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]
Reporter | ||
Updated•10 years ago
|
QA Contact: aaron.train
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8365779 -
Flags: review?(margaret.leibovic)
Comment 4•10 years ago
|
||
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+
Updated•10 years ago
|
Assignee: nobody → maxli
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 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+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
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]
Comment 8•10 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•