If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in 2.2 S3 (9jan)

Status

Firefox OS
Gaia::System::Window Mgmt
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
* 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
(Assignee)

Updated

3 years ago
Blocks: 1023688
(Assignee)

Updated

3 years ago
Priority: -- → P2
take it to analyze gaia code first.
Assignee: nobody → gduan
I found no clue for this bug.

Hi Peter,
could you also take a look on it? I have no clue on it.
Assignee: gduan → nobody
Flags: needinfo?(pchang)
(Assignee)

Comment 3

3 years ago
Already setup this bug as P2, will work on this later
Flags: needinfo?(pchang)
(Assignee)

Updated

3 years ago
Assignee: nobody → pchang

Updated

3 years ago
Summary: [Text Selection] flicker when selection range is changed → [Text Selection] flicker to tilt mode when selection range is changed
(Assignee)

Comment 4

3 years ago
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.
Attachment #8535415 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8535415 - Flags: review?(roc) → review?(ehsan.akhgari)
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?
Attachment #8535415 - Flags: review?(ehsan.akhgari) → review?(roc)
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.
Attachment #8535415 - Flags: review?(roc) → review-
(Assignee)

Comment 7

3 years ago
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;
(Assignee)

Comment 8

3 years ago
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.
Attachment #8535415 - Attachment is obsolete: true
Attachment #8538995 - Flags: review?(roc)
(Assignee)

Comment 9

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

Remove the redundant spaces.
Attachment #8538995 - Attachment is obsolete: true
Attachment #8538995 - Flags: review?(roc)
Attachment #8538997 - Flags: review?(roc)
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"
Attachment #8538997 - Flags: review?(roc) → review+
(Assignee)

Comment 11

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

Update comment as suggest.
Attachment #8538997 - Attachment is obsolete: true
Attachment #8539880 - Flags: review+
(Assignee)

Comment 12

3 years ago
Try server result is positive.

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

Comment 13

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/155b9d4cb8cb
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
(Assignee)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
(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
(Assignee)

Comment 17

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

Fix marionette failure.
Attachment #8539880 - Attachment is obsolete: true
Attachment #8540234 - Flags: review+
(Assignee)

Comment 18

3 years ago
wait for marionette testing result.

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

Comment 19

3 years ago
Marionette tests are passed.
Keywords: checkin-needed
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"
https://hg.mozilla.org/integration/b2g-inbound/rev/003d931d33e5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/003d931d33e5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
You need to log in before you can comment on or make changes to this bug.