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)

defect
Not set
normal

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.
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
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)
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
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+
(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.
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.
Attachment #8743506 - Flags: review?(mats) → review+
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?
(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. :-)
Attachment #8743506 - Flags: review?(tlin) → review+
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: nobody → markcapella
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/adc7dd34508b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(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)
See Bug 1266782 for followup.
Flags: needinfo?(markcapella)
Depends on: 1267929
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.