[Text Selection] Selection carets does not update its position when selecting text in an unfocused content editable area

RESOLVED FIXED in Firefox 38

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

37 Branch
mozilla38
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master verified)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Description: When the cursor is on the To field and the user selects text in the message body, the text will highlight but the selection carets are wrongly 
positioned. 

### Repro Steps:
1. Open Messages app
2. Click pencil icon on the upper-right corner to create a new message
3. Type characters in 10+ rows at least in the message body so that the scroll bar will show up, then scroll down to the bottom of message body
4. Tap on To field to move focus
5. Long tap to select a word in message body
6. Focus is changed to message body, which triggers word suggestion bar popping 
out and a little bit scrolling.

### Expected:
1. Selection carets should be updated to the right positions along with selected 
word.

### Actual:
1. Selection carets are not updated to the right positions after scrolling.

### Reproduce rate
always

Note: 
This issue occurs after we apply the patch for Bug-1110917, since the patch
fixes the focus change problem and do change focus from To field to message 
body.
Assignee: nobody → jeremychen
If ScrollPositionChanged is called under APZ-enable mode, but APZ is not started yet, ScrollPositionChanged should update positions of selection carets by itself.
Attachment #8547880 - Flags: review?(roc)
Status: NEW → ASSIGNED
Comment on attachment 8547880 [details] [diff] [review]
Part1: handle scroll position change that does not triggered by APZ v1

Review of attachment 8547880 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +1219,5 @@
> +    } else {
> +      // TODO: When async pan zoom is enabled, we still need to handle the scroll
> +      // position changed that does not trigger by APZ. Otherwise, the position of
> +      // SelectionCarets might be wrong. For example, in SMS app, long press on To
> +      // field, then long press on the editing field.

I think this TODO is fulfilled, and should be removed. It will be even better to rephrase it and put it into the commit message.
Attachment #8547880 - Flags: feedback+
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)

Thanks for the advice. I'll remove it and upload patch v2.
Remove TODO comment that has already been fulfilled in this patch.
Attachment #8547880 - Attachment is obsolete: true
Attachment #8547880 - Flags: review?(roc)
Attachment #8547919 - Flags: review?(roc)
Priority: -- → P2
Comment on attachment 8547919 [details] [diff] [review]
Part1: handle scroll position change that is not triggered by APZ v2

Review of attachment 8547919 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +77,5 @@
>    , mActiveTouchId(-1)
>    , mCaretCenterToDownPointOffsetY(0)
>    , mDragMode(NONE)
>    , mAsyncPanZoomEnabled(false)
> +  , mAsyncPanZoomStarted(false)

Call it mInAsyncPanZoomGesture

@@ +1216,5 @@
>  
> +      SELECTIONCARETS_LOG("Launch scroll end detector");
> +      LaunchScrollEndDetector();
> +    } else {
> +      if(!mAsyncPanZoomStarted) {

Space before (
Attachment #8547919 - Flags: review?(roc) → review+
Rename flag to mInAsyncPanZoomGesture and add a space between "if" and "(".
Attachment #8547919 - Attachment is obsolete: true
This bug can only be reproduced under APZ-enable mode. So, I write a Gaia JS integration test case, which can be run on device with APZ-enable mode.
Attachment #8555671 - Flags: review?(gduan)
Triage: feature blocker
blocking-b2g: --- → 2.2+
Comment on attachment 8549448 [details] [diff] [review]
Part1: handle scroll position change that is not triggered by APZ v3 (carry r+: roc)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Text selection/copy/paste support on B2G
User impact if declined: Selection carets may be wrongly positioned, so users may not drag the carets to change selection range.
Testing completed: 
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch:none
Attachment #8549448 - Flags: review+
Attachment #8549448 - Flags: approval-mozilla-b2g37?
Comment on attachment 8555671 [details]
Part2: Gaia JS integration test case (PR to master)

plz see comments in github. some nits need your help to modify before landing.
Attachment #8555671 - Flags: review?(gduan)
(In reply to George Duan [:gduan] [:喬智] from comment #15)
> Comment on attachment 8555671 [details]
> Part2: Gaia JS integration test case (PR to master)
> 
> plz see comments in github. some nits need your help to modify before
> landing.

I've modified the patch and push it on github. Please check again, thx :)
Enlarge scrollTop offsets to raise robustness. Modify test assertion criterion to compare the selectedContent before/after draging selection caret.
Attachment #8555671 - Attachment is obsolete: true
Attachment #8556314 - Flags: review?(gduan)
Comment on attachment 8556314 [details] [review]
Part2: Gaia JS integration test case v2 (PR to master)

LGTM, r=gduan
Attachment #8556314 - Flags: review?(gduan) → review+
Remove a trailing whitespace.
Attachment #8556314 - Attachment is obsolete: true
Attachment #8556898 - Flags: review+
Master: https://github.com/mozilla-b2g/gaia/commit/45475198737a504d81932a9c90002902054fce23
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8549448 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0675625fe7dd

The test needs rebasing for v2.2 uplift (if you want it there at all).
Flags: needinfo?(jeremychen)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0675625fe7dd
> 
> The test needs rebasing for v2.2 uplift (if you want it there at all).

Thx for reminding. We're not planning to put the test in v2.2.
Flags: needinfo?(jeremychen)
This bug has been verified as "pass" on latest Nightly build of Flame master by the STR in Comment 0.

Actual results: Selection carets will be updated to the right positions along with selected. 
word.
See attachment: verified_master.3gp
Reproduce rate: 0/10


Device: Flame master (Verified)
Build ID               20150705160206
Gaia Revision          dc6c18c0dea7af3c40bfff86c530fd877d899dc4
Gaia Date              2015-07-04 01:35:20
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/136c41fca853
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150705.193055
Firmware Date          Sun Jul  5 19:31:07 EDT 2015
Bootloader             L1TC000118D0
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.