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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Jeremy Chen ([e-mail])]

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.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: repro video clip

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Part1: handle scroll position change that does not triggered by APZ v1

If ScrollPositionChanged is called under APZ-enable mode, but APZ is not started yet, ScrollPositionChanged should update positions of selection carets by itself.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 4

[Jeremy Chen ([e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3)

Thanks for the advice. I'll remove it and upload patch v2.

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Part1: handle scroll position change that is not triggered by APZ v2

Remove TODO comment that has already been fulfilled in this patch.

Comment 6

[Jeremy Chen ([e-mail])]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc03e985e3a3

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 (

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: Part1: handle scroll position change that is not triggered by APZ v3 (carry r+: roc)

Rename flag to mInAsyncPanZoomGesture and add a space between "if" and "(".

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Part2: Gaia JS integration test case (PR to master)

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.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Nice!

Comment 11

[howie [:howie]]

Triage: feature blocker

Comment 12

[Jeremy Chen ([e-mail])]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b47c5f0dd4

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9b47c5f0dd4

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

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.

Comment 16

[Jeremy Chen ([e-mail])]

(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 :)

Comment 17

[Jeremy Chen ([e-mail])]

Attachment: Part2: Gaia JS integration test case v2 (PR to master)

Enlarge scrollTop offsets to raise robustness. Modify test assertion criterion to compare the selectedContent before/after draging selection caret.

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8556314 [details] [review]
Part2: Gaia JS integration test case v2 (PR to master)

LGTM, r=gduan

Comment 19

[Jeremy Chen ([e-mail])]

Attachment: Part2: Gaia JS integration test case v3 (PR to master) (carry r+: gduan)

Remove a trailing whitespace.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/45475198737a504d81932a9c90002902054fce23

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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).

Comment 22

[Jeremy Chen ([e-mail])]

(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.

Comment 23

[Shally Li（leave from Mozilla）]

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

Comment 24

[Shally Li（leave from Mozilla）]

Attachment: verified_master.3gp