Closed
Bug 1265750
Opened 8 years ago
Closed 8 years ago
Some word cannot be selected via long pressing
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Firefox for Android Graveyard
Text Selection
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: TYLin, Assigned: capella)
References
(Blocks 1 open bug, )
Details
Attachments
(2 files)
Steps to reproduce: 1. Open data:text/html,<input value="123"><p>456</p> on Fennec. (To test on desktop, change "layout.accessiblecaret.extend_selection_for_phone_number" to true. Need to manually add the pref before bug 1265695 is fixed.) 2. Long press to select "456". Expected result: "456" is selected. Actual result: Nothing is selected. This bug can be reproduced on both gecko carets and the old java carets.
Reporter | ||
Comment 1•8 years ago
|
||
The root cause for this bug is that after calling selection->Modify("extend", "backward", "character") in [1], both anchor and focus node will be changed. Down to the callstack in [2], the range will be collapsed when extending the selection from "456" (backward) to the boundary of <input>. So the backout logic in [3] is not correct since the anchor node is changed. I think we'll need to create a clone for the anchor focus range, and restore it later. [1] https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/base/AccessibleCaretManager.cpp#860,875-877 [2] https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/dom/base/nsRange.cpp#1230-1235 [3] https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/layout/base/AccessibleCaretManager.cpp#893-894
Reporter | ||
Comment 2•8 years ago
|
||
Mark, I don't feel we can use the old java phone selection logic because I can reproduce this bug on release (Firefox 45). I hope this patch fixes all the edge cases you found :)
Attachment #8742895 -
Flags: feedback?(markcapella)
Assignee | ||
Comment 3•8 years ago
|
||
Ah, I do see it in the old Java version, because we restore the selection assuming the anchorNode doesn't change. So, in the worst case situation of [0], longpress |456| where the surrounding field is an <input>, selecting forward and backwards, the selection /changes to a null in "between" the fields/ and both the anchorNode and focusNode change. Your code addresses this properly, though I wonder if that's a basic nsISelection bug and we might file (?) I'd not expect the |extend()| selection method to completely start what amounts to a new selection. In a similar situation [1], longpress |456|, the process is different but still handled correctly. We'll still have the second bug [2] unless you update your patch to add the final |SetSelectionDirection(eDirNext);| [0] data:text/html,<input value="123"><p>456</p><input value="123"> selectedText initial: |456| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : || anchorNode=anchorNodeOrig [FALSE] focusNode=focusNodeOrig [FALSE] selectedText final r: |456| selectedText initial: |456| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : || anchorNode=anchorNodeOrig [FALSE] focusNode=focusNodeOrig [FALSE] selectedText final r: |456| [1] data:text/html,123<em> 456 </em>78 selectedText initial: |456| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : |456 | anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : |456 7| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText now is : |456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText now is : |456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText final =: |456 78| selectedText initial: |456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : | 456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [TRUE ] selectedText now is : |3 456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText now is : |23 456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText now is : |123 456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText now is : |123 456 78| anchorNode=anchorNodeOrig [TRUE ] focusNode=focusNodeOrig [FALSE] selectedText final =: |123 456 78| [2] https://www.dropbox.com/s/rrdgqbnmczbyd7g/bugJumpyPhone.mp4?dl=0
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8742895 [details] [diff] [review] Use old anchor focus range to restore from previous extending selection. (v1) Review of attachment 8742895 [details] [diff] [review]: ----------------------------------------------------------------- So feedback+, with an updated testAccessibleCarets test we should be good to go.
Attachment #8742895 -
Flags: feedback?(markcapella) → feedback+
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Mark Capella [:capella] from comment #3) > Ah, I do see it in the old Java version, because we restore the selection > assuming the anchorNode doesn't change. > > So, in the worst case situation of [0], longpress |456| where the > surrounding field is an <input>, selecting forward and backwards, the > selection /changes to a null in "between" the fields/ and both the > anchorNode and focusNode change. Your code addresses this properly, though I > wonder if that's a basic nsISelection bug and we might file (?) > > I'd not expect the |extend()| selection method to completely start what > amounts to a new selection. I agree that changing the anchor node after extending the selection look like a bug to me. It's better to compare what Chrome does here. For example, manually backward select "456" in the my test case, and calling window.getSelection().modify("extend", "backward", "character") and see how the range in window.getSelection() changed.
Assignee | ||
Comment 6•8 years ago
|
||
TYLin, I'm cc:ing mats now, before I post a patch (with testcase) so he can tune into the conversation. Testing in Chrome is a little bit apple-oranges kinda thing, but here's a quick fiddle I put together https://jsfiddle.net/yLr3sttk/ It seems our problem is that extending the reversed selection "backward" (left in a LTR <p>) a char at a time towards a previous <input> runs into its anonymous structure and causes this issue. <input> <div> (anonymous) <#text> (anonymous) Chrome seems to handle it gracefully, so perhaps there's something we can improve here.
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47859/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47859/
Attachment #8743506 -
Flags: review?(tlin)
Attachment #8743506 -
Flags: review?(mats)
Updated•8 years ago
|
Attachment #8743506 -
Flags: review?(mats) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8743506 [details] MozReview Request: Bug 1265750 - Some word cannot be selected via long pressing, r=TYLin, mats https://reviewboard.mozilla.org/r/47859/#review44689 Both comments are already existing problems, but please file a follow-up bug as needed. Your changes looks good to me. ::: layout/base/AccessibleCaretManager.cpp:871 (Diff revision 1) > AccessibleCaretManager::ExtendPhoneNumberSelection(const nsAString& aDirection) const > { > nsIDocument* doc = mPresShell->GetDocument(); > > // Extend the phone number selection until we find a boundary. > Selection* selection = GetSelection(); Does the Modify call dispatch selection events? If so, using a raw pointer here seems unsafe. |doc| should probably be a strong ref as well for the same reason. ::: layout/base/AccessibleCaretManager.cpp:873 (Diff revision 1) > nsIDocument* doc = mPresShell->GetDocument(); > > // Extend the phone number selection until we find a boundary. > Selection* selection = GetSelection(); > > while (selection) { Why a while-loop? I don't see that |selection| changes in this loop, so we could use an 'if' instead to be less confusing?
Comment 9•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #8) > Why a while-loop? Ah, now I got what this loop is doing. So we're extending one character at a time for as long it matches a "phone number" regexp. OK. I guess I should read the existing comments more carefully. :-)
Reporter | ||
Updated•8 years ago
|
Attachment #8743506 -
Flags: review?(tlin) → review+
Reporter | ||
Comment 10•8 years ago
|
||
Comment on attachment 8743506 [details] MozReview Request: Bug 1265750 - Some word cannot be selected via long pressing, r=TYLin, mats https://reviewboard.mozilla.org/r/47859/#review44763 r=me
Assignee | ||
Comment 11•8 years ago
|
||
Quick TRY push https://treeherder.mozilla.org/#/jobs?repo=try&revision=92a412ed5ddaf6ec3df365196226eb6163caee07
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adc7dd34508b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 15•8 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #8) > Does the Modify call dispatch selection events? > If so, using a raw pointer here seems unsafe. > |doc| should probably be a strong ref as well for the same reason. Don't forget to investigate / file a follow-up bug for that issue if needed.
Flags: needinfo?(markcapella)
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
•