Drag selection monocles should not push other monocles out of the way

RESOLVED FIXED in Firefox 30

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jimm, Assigned: azasypkin)

Tracking

Trunk
Firefox 30
x86_64
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [selection] p=5 s=it-30c-29a-28b.3 r=ff30 [qa-])

Attachments

(3 attachments, 7 obsolete attachments)

2.56 KB, patch
jimm
: review+
Details | Diff | Splinter Review
24.93 KB, patch
azasypkin
: review+
Details | Diff | Splinter Review
10.06 KB, patch
jimm
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
To better match Win8 and android behavior.

str:

1) select some text
2) drag one monocle toward the other

result: the other monocle gets push out of the way

desired: start and end nodes should reverse.
(Reporter)

Updated

6 years ago
Duplicate of this bug: 904947
(Reporter)

Updated

5 years ago
Duplicate of this bug: 872620
(Reporter)

Updated

5 years ago
Blocks: 957244
(Reporter)

Updated

5 years ago
OS: Windows 8 Metro → Windows 8.1
Blocks: 904960
Whiteboard: [selection] → [selection] p=0
Priority: -- → P1
Whiteboard: [selection] p=0 → [selection] p=5
Assignee: nobody → azasypkin
Blocks: metrobacklog
Status: NEW → ASSIGNED
QA Contact: kamiljoz
Whiteboard: [selection] p=5 → [selection] p=5 s=it-30c-29a-28b.3 r=ff30
(Reporter)

Comment 3

5 years ago
Getting this fixed should solve a number of problems. One annoyance I keep running into is that sometimes when you try to drag a single monocle when both monocles are close, you end you end up dragging both. This happens often when transitioning from caret to selection mode.

I'm hoping this will address the issue by allowing the monocle you actually grab to transition across the other. Monocle z-level probably plays a role here.
Thanks for the input! Yes, I'm trying to cover as much cases as possible. A bunch of different selection issues are related to this in one or another way.
Initial patch version. Still doesn't work right for inputs with out-of-bound selection.
Added some WIP patches, still working on out-of-bound selection for inputs and some other oddities.
Still poking around _handleSelectionPoint trying to improve and simplify it. Also have problems with textareas. Probably it can be addressed in the separate bug.

Patch is mainly oriented on inputs as I have several dependent bugs.
Attachment #8386052 - Attachment is obsolete: true
Attachment #8387403 - Flags: feedback?(jmathies)
(Reporter)

Comment 11

5 years ago
This is acting kind of strange in text areas - 

1) off an initial drag, I always seem to select from the caret position down to the the line below the caret position.

2) selection seems to jump around selecting larger blocks that I'm aiming for. 

3) dragging across the other monocle doesn't reverse selection, it still seems to drag the other monocle with it resulting in a single character of selection dragging under my finger.
Attachment #8387403 - Attachment is obsolete: true
Attachment #8387403 - Flags: feedback?(jmathies)
Attachment #8388696 - Flags: feedback?(jmathies)
Attachment #8387405 - Attachment is obsolete: true
Attachment #8387405 - Flags: feedback?(jmathies)
Attachment #8388705 - Flags: feedback?(jmathies)
(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #11)
> This is acting kind of strange in text areas - 
> 
> 1) off an initial drag, I always seem to select from the caret position down
> to the the line below the caret position.
> 
> 2) selection seems to jump around selecting larger blocks that I'm aiming
> for. 
> 
> 3) dragging across the other monocle doesn't reverse selection, it still
> seems to drag the other monocle with it resulting in a single character of
> selection dragging under my finger.

Thanks for checking this out! Yeah as I mentioned textareas have a bunch of problems. I've tried to improve swapping for general text and textareas, but it's still not as precise as I want it to be. I think it would be better to file follow ups to cover every corner case + polish it if required.
(Reporter)

Comment 17

5 years ago
Comment on attachment 8388696 [details] [diff] [review]
Part 1: Swapping start and end monocles when they overlap

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

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +137,5 @@
>    get dragging() {
>      return this._element.customDragger.dragging;
>    },
>  
> +  get restrictedToBounds() {

can you add a comment here explaining what this flag does?

@@ +947,5 @@
>  
> +  _selectionSwap: function _selectionSwap() {
> +    [this.startMark.tag, this.endMark.tag] = [this.endMark.tag,
> +        this.startMark.tag];
> +    [this._startMark, this._endMark] = [this.endMark, this.startMark];

nice!
Attachment #8388696 - Flags: feedback?(jmathies) → review+
(Reporter)

Updated

5 years ago
Attachment #8388701 - Flags: feedback?(jmathies) → review+
(Reporter)

Comment 18

5 years ago
Comment on attachment 8388705 [details] [diff] [review]
Part 3: Mochitests for content and chrome input selection.

I got failures in both of these running them locally, but it looks like your test push to try came up green. Have you had any trouble running these locally?
(Reporter)

Comment 19

5 years ago
Comment on attachment 8388705 [details] [diff] [review]
Part 3: Mochitests for content and chrome input selection.

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

::: browser/metro/base/tests/mochitest/browser_selection_inputs.js
@@ +221,5 @@
> +    let touchDrag = new TouchDragAndHold();
> +    yield touchDrag.start(gWindow, startXPos, startYPos, 320, startYPos);
> +
> +    yield waitForCondition(() => getTrimmedSelection(gInput).toString() ==
> +        "straight on like", kCommonWaitMs, kCommonPollMs);

here I got "straight on lik" and the test times out.

::: browser/metro/base/tests/mochitest/browser_selection_urlbar.js
@@ +341,5 @@
> +
> +    // Place caret to the input start
> +    sendTap(window, selectionRectangle.left, selectionRectangle.top);
> +    yield waitForCondition(() => !SelectionHelperUI.isSelectionUIVisible &&
> +        SelectionHelperUI.isCaretUIVisible);

this timed out for me locally.
Attachment #8388705 - Attachment is obsolete: true
Attachment #8388705 - Flags: feedback?(jmathies)
Attachment #8389120 - Flags: review?(jmathies)
Thanks for review!

(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #17)
> Comment on attachment 8388696 [details] [diff] [review]
> Part 1: Swapping start and end monocles when they overlap
> 
> Review of attachment 8388696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/helperui/SelectionHelperUI.js
> @@ +137,5 @@
> >    get dragging() {
> >      return this._element.customDragger.dragging;
> >    },
> >  
> > +  get restrictedToBounds() {
> 
> can you add a comment here explaining what this flag does?

Done!


(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #19)
> Comment on attachment 8388705 [details] [diff] [review]
> Part 3: Mochitests for content and chrome input selection.
> 
> Review of attachment 8388705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/tests/mochitest/browser_selection_inputs.js
> @@ +221,5 @@
> > +    let touchDrag = new TouchDragAndHold();
> > +    yield touchDrag.start(gWindow, startXPos, startYPos, 320, startYPos);
> > +
> > +    yield waitForCondition(() => getTrimmedSelection(gInput).toString() ==
> > +        "straight on like", kCommonWaitMs, kCommonPollMs);
> 
> here I got "straight on lik" and the test times out.
> 
> ::: browser/metro/base/tests/mochitest/browser_selection_urlbar.js
> @@ +341,5 @@
> > +
> > +    // Place caret to the input start
> > +    sendTap(window, selectionRectangle.left, selectionRectangle.top);
> > +    yield waitForCondition(() => !SelectionHelperUI.isSelectionUIVisible &&
> > +        SelectionHelperUI.isCaretUIVisible);
> 
> this timed out for me locally.

Hmm, didn't have problems on my main Lenovo W530, but just run tests on Surface and noticed the same failures. Looks like touchAndDrag accuracy depends on resolution, so I just simplified tests, but it shouldn't be a problem for the user. But that is strange anyway, I thought setDevPixelEqualToPx() call should have helped.

Pushed updated tests to try: https://tbpl.mozilla.org/?tree=Try&rev=139c9df0e673
(Reporter)

Updated

5 years ago
Attachment #8389120 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/fx-team/rev/3c9c98992742
https://hg.mozilla.org/integration/fx-team/rev/1b636faa3187
https://hg.mozilla.org/integration/fx-team/rev/b1b516d5c5e6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [selection] p=5 s=it-30c-29a-28b.3 r=ff30 → [selection] p=5 s=it-30c-29a-28b.3 r=ff30[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3c9c98992742
https://hg.mozilla.org/mozilla-central/rev/1b636faa3187
https://hg.mozilla.org/mozilla-central/rev/b1b516d5c5e6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [selection] p=5 s=it-30c-29a-28b.3 r=ff30[fixed-in-fx-team] → [selection] p=5 s=it-30c-29a-28b.3 r=ff30
Target Milestone: --- → Firefox 30
Blocks: 881938
Blocks: 881932
Whiteboard: [selection] p=5 s=it-30c-29a-28b.3 r=ff30 → [selection] p=5 s=it-30c-29a-28b.3 r=ff30 [qa-]
You need to log in before you can comment on or make changes to this bug.