1068474 - [Text Selection] flicker to tilt mode when selection range is changed

Status: RESOLVED FIXED

Description

[Peter Chang[:pchang]]

* Description:
 flicker happens when drag selection caret to change selection range  


* Reproduction steps:
  1.Open message app
  2. Type a long word on 'To' filed
  3. Drag the right of seleciton caret to change selection rang
  4.Check the selection range

* Expected result:
No flicker
* Actual result: 
flicker happens on the selection range

Comment 1

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

take it to analyze gaia code first.

Comment 2

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

I found no clue for this bug.

Hi Peter,
could you also take a look on it? I have no clue on it.

Comment 3

[Peter Chang[:pchang]]

Already setup this bug as P2, will work on this later

Comment 4

[Peter Chang[:pchang]]

Created attachment 8535415 [details] [diff] [review]
limit selection range based on the boundary of start/end caret

The patch is try to improve the user experience for selection drag.

Selection is easy to becomes one character only when you drag down the start caret, because the drag position couldn't behind the end caret. From user point of view, it doesn't provide the good user experience. With this path, the selection rang won't change if you only drag down the start caret vertically.

Comment 5

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Comment on attachment 8535415 [details] [diff] [review]
limit selection range based on the boundary of start/end caret

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

I didn't really review these parts.  roc, do you mind taking this, please?

Comment 6

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

Comment on attachment 8535415 [details] [diff] [review]
limit selection range based on the boundary of start/end caret

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

::: layout/base/SelectionCarets.h
@@ +245,5 @@
> +  // The Y boundary of the first selected frame, and is used to limit the
> +  // selection not above this boundary during drag up the EndCaret
> +  nscoord mDragUpYBoundary;
> +  // The Y boundary of the last selected frame, and is used to limit the
> +  // selection not below this boundary during drag down the StartCaret

This text doesn't quite make sense. Please explain what exactly you mean by "Y boundary" and what limitation is applied.

Comment 7

[Peter Chang[:pchang]]

Comment on attachment 8535415 [details] [diff] [review]
limit selection range based on the boundary of start/end caret

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

::: layout/base/SelectionCarets.h
@@ +245,5 @@
> +  // The Y boundary of the first selected frame, and is used to limit the
> +  // selection not above this boundary during drag up the EndCaret
> +  nscoord mDragUpYBoundary;
> +  // The Y boundary of the last selected frame, and is used to limit the
> +  // selection not below this boundary during drag down the StartCaret

troc, current selectioncarets drag behavior is that users can't drag the start caert down across the end caret and drag the end caret up over the start caret. If users do, the selection range will be selected one character only. Imagine users try to drag the selection horizontally but it is easy to be treat as across the start/end caret if your finger is above/below the selection line. And you might see the selection range change to one line or one character which causes bad user experience. Therefore, this patch is try to change the position of touch input to improve the user behavior.

roc, how about the comment below? Does it make sense to you?

// The horizontal boundary is defined by the first selected frame which determines the startcaret position. When users drag the endcaert up, the touch input(pos.y) will be changed not to across this boundary. Otherwise, the selection range will change to one character only which casues the bad user experience.
nscoord mDragUpYBoundary;

// The horizontal boundary is defined by the last selected frame which determines the endcaret position. When users drag the startcaert down, the touch input(pos.y) will be changed not to across this boundary. Otherwise, the selection range will change to one character only which casues the bad user experience.
nscoord mDragDownYBoundary;

Comment 8

[Peter Chang[:pchang]]

Created attachment 8538995 [details] [diff] [review]
v2 limit selection range based on the boundary of start/end caret

roc, I just update the comment as you suggest and explain more detail in comment 7.

Comment 9

[Peter Chang[:pchang]]

Created attachment 8538997 [details] [diff] [review]
v3 limit selection range based on the boundary of start/end caret

Remove the redundant spaces.

Comment 10

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

Comment on attachment 8538997 [details] [diff] [review]
v3 limit selection range based on the boundary of start/end caret

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

This code assumes lines are horizontal. But soon we'll enable vertical text, which means lines, even editable lines and text fields, can be vertical. So this will need to be changed. But I guess a lot of this code will need to be changed for that.

::: layout/base/SelectionCarets.h
@@ +242,5 @@
>  
>    nscoord mCaretCenterToDownPointOffsetY;
> +
> +  // The horizontal boundary is defined by the first selected frame which
> +  // determines the startcaret position. When users drag the endcaert up,

"end-caret"

@@ +243,5 @@
>    nscoord mCaretCenterToDownPointOffsetY;
> +
> +  // The horizontal boundary is defined by the first selected frame which
> +  // determines the startcaret position. When users drag the endcaert up,
> +  // the touch input(pos.y) will be changed not to across this boundary.

"to not cross"

@@ +248,5 @@
> +  // Otherwise, the selection range changes to one character only
> +  // which causes the bad user experience.
> +  nscoord mDragUpYBoundary;
> +  // The horizontal boundary is defined by the last selected frame which
> +  // determines the endcaret position. When users drag the startcaert down,

"start-caret"

@@ +249,5 @@
> +  // which causes the bad user experience.
> +  nscoord mDragUpYBoundary;
> +  // The horizontal boundary is defined by the last selected frame which
> +  // determines the endcaret position. When users drag the startcaert down,
> +  // the touch input(pos.y) will be changed not to across this boundary.

"to not cross"

Comment 11

[Peter Chang[:pchang]]

Created attachment 8539880 [details] [diff] [review]
v4 limit selection range based on the boundary of start/end caret

Update comment as suggest.

Comment 12

[Peter Chang[:pchang]]

Try server result is positive.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fb9ce30f125

Comment 13

[Peter Chang[:pchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/155b9d4cb8cb

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ee1857733 for Marionette failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4857285&repo=mozilla-inbound

Comment 15

[Peter Chang[:pchang]]

(In reply to Phil Ringnalda (:philor) from comment #14)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ee1857733 for
> Marionette failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=4857285&repo=mozilla-
> inbound

I think right now the selecitoncaert on firefox has some problem.
If I select one word by double click, the selection carets won't change after drag.
But it changes if I select one word by flicking. The problem of double click is caused by no selectionchanges after dragging.

This issues may cause some side effect on my modification.

Comment 16

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang PTO from 12/23 to 1/4][:peter] from comment #15)
> (In reply to Phil Ringnalda (:philor) from comment #14)
> > Backed out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/a87ee1857733 for
> > Marionette failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=4857285&repo=mozilla-
> > inbound
> 
> I think right now the selecitoncaert on firefox has some problem.
> If I select one word by double click, the selection carets won't change
> after drag.
> But it changes if I select one word by flicking. The problem of double click
> is caused by no selectionchanges after dragging.
> 
> This issues may cause some side effect on my modification.

It works well to drag the word which is selected by long tap in browser, but it doesn't work when the word is selected by double click. But after I skip the call in [1], it works now. But I already confirm this issue is not related to the marionette failures in comment 14. 

[1]http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp?from=nsSelection.cpp&case=true#1228

Comment 17

[Peter Chang[:pchang]]

Created attachment 8540234 [details] [diff] [review]
v5 limit selection range based on the boundary of start/end caret

Fix marionette failure.

Comment 18

[Peter Chang[:pchang]]

wait for marionette testing result.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=afa630aeff02

Comment 19

[Peter Chang[:pchang]]

Marionette tests are passed.

Comment 20

[Ting-Yu Lin [:TYLin] (UTC+8)]

Comment on attachment 8540234 [details] [diff] [review]
v5 limit selection range based on the boundary of start/end caret

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

::: layout/base/SelectionCarets.h
@@ +242,5 @@
>  
>    nscoord mCaretCenterToDownPointOffsetY;
> +
> +  // The horizontal boundary is defined by the first selected frame which
> +  // determines the star-tcaret position. When users drag the end-caert up,

Please fix the typo "star-tcaret" and "end-caert" here.

@@ +248,5 @@
> +  // Otherwise, the selection range changes to one character only
> +  // which causes the bad user experience.
> +  nscoord mDragUpYBoundary;
> +  // The horizontal boundary is defined by the last selected frame which
> +  // determines the end-caret position. When users drag the start-caert down,

"start-caret"

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/003d931d33e5

Comment 22

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/003d931d33e5