Closed Bug 1068474 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(1 file, 4 obsolete files)

* 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
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)
Already setup this bug as P2, will work on this later
Flags: needinfo?(pchang)
Assignee: nobody → pchang
Summary: [Text Selection] flicker when selection range is changed → [Text Selection] flicker to tilt mode when selection range is changed
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)
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-
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;
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)
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+
Update comment as suggest.
Attachment #8538997 - Attachment is obsolete: true
Attachment #8539880 - Flags: review+
(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.
(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
Fix marionette failure.
Attachment #8539880 - Attachment is obsolete: true
Attachment #8540234 - Flags: review+
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/mozilla-central/rev/003d931d33e5
Status: NEW → RESOLVED
Closed: 6 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.