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)
Tracking
(Not tracked)
RESOLVED
FIXED
2.2 S3 (9jan)
People
(Reporter: pchang, Assigned: pchang)
References
Details
Attachments
(1 file, 4 obsolete files)
3.85 KB,
patch
|
pchang
:
review+
|
Details | Diff | Splinter Review |
* 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•7 years ago
|
Blocks: CopyPasteLegacy
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 2•7 years ago
|
||
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•6 years ago
|
||
Already setup this bug as P2, will work on this later
Flags: needinfo?(pchang)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → pchang
Updated•6 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•6 years ago
|
||
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•6 years ago
|
Attachment #8535415 -
Flags: review?(roc) → review?(ehsan.akhgari)
Comment 5•6 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]: ----------------------------------------------------------------- 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•6 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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Update comment as suggest.
Attachment #8538997 -
Attachment is obsolete: true
Attachment #8539880 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
Try server result is positive. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fb9ce30f125
Assignee | ||
Comment 13•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/155b9d4cb8cb
Comment 14•6 years ago
|
||
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•6 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•6 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•6 years ago
|
||
Fix marionette failure.
Attachment #8539880 -
Attachment is obsolete: true
Attachment #8540234 -
Flags: review+
Assignee | ||
Comment 18•6 years ago
|
||
wait for marionette testing result. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=afa630aeff02
Comment 20•6 years ago
|
||
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•6 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/003d931d33e5
Keywords: checkin-needed
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.
Description
•