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

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P1)

Description

[Jim Mathies [:jimm]]

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.

Comment 3

[Jim Mathies [:jimm]]

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.

Comment 4

[Oleg Zasypkin [:azasypkin]]

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.

Comment 5

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 1: Swapping start and end monocles when they overlap (wip).

Initial patch version. Still doesn't work right for inputs with out-of-bound selection.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 2: Don't use selectAtPoint for inputs as we already have collapsed one (caret).

Comment 7

[Oleg Zasypkin [:azasypkin]]

Added some WIP patches, still working on out-of-bound selection for inputs and some other oddities.

Comment 8

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 1: Swapping start and end monocles when they overlap

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.

Comment 9

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 2: Don't use selectAtPoint for inputs as we already have collapsed one while caret-to-select switch

Comment 10

[Oleg Zasypkin [:azasypkin]]

Attachment: Part3: Mochitests for content and chrome input selection.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8246b36a0d9a

Comment 11

[Jim Mathies [:jimm]]

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.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 1: Swapping start and end monocles when they overlap

Comment 13

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 2: Don't use selectAtPoint for inputs as we already have collapsed one while caret-to-select switch

Comment 14

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 3: Mochitests for content and chrome input selection.

Comment 15

[Oleg Zasypkin [:azasypkin]]

(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.

Comment 16

[Oleg Zasypkin [:azasypkin]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=66cbf7c9d49f

Comment 17

[Jim Mathies [:jimm]]

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!

Comment 18

[Jim Mathies [:jimm]]

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?

Comment 19

[Jim Mathies [:jimm]]

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.

Comment 20

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 1: Swapping start and end monocles when they overlap

Comment 21

[Oleg Zasypkin [:azasypkin]]

Attachment: Part 3: Mochitests for content and chrome input selection

Comment 22

[Oleg Zasypkin [:azasypkin]]

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

Comment 23

[Oleg Zasypkin [:azasypkin]]

Try tests have passed: https://tbpl.mozilla.org/?tree=Try&rev=139c9df0e673

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c9c98992742
https://hg.mozilla.org/mozilla-central/rev/1b636faa3187
https://hg.mozilla.org/mozilla-central/rev/b1b516d5c5e6