1140238 - Selection range disappears after dragging SelectionCarets across non-selectable elements

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Steps to reproduce:
1. Open about:config, enable "selectioncaret.enabled", and restart browser.
2. Open https://hg.mozilla.org/mozilla-central/raw-file/1968388d42c2/testing/marionette/client/marionette/www/test_selectioncarets_multiplerange.html
3. Drag a selection which is across a non-selectable elements. For example, drag a  selection from beginning of "user can select this 3" to the end of "user can select this 4".
4. Drag the left SelectionCarets arbitrarily.

Actual results:
The selection range on "user can select this 4" disappears, and the right SelectionCarets jump to the incorrect position.

Expected results:
The selection range on "user can select this 4" maintains, and the right SelectionCarets remains at its position.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 1 - Add a test to drag first caret over non-selectable.

The original test had dragged the second SelectionCaret over
non-selectable elements. After that, we should further drag the first
SelectionCaret over non-selectable elements, and confirm that the
selection ranges are correct.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Change the opposite range as generated when dragging caret.

When dragging the second caret (eDirNext) over non-selectable elements,
the last range is not generated. Therefore when dragging the first
caret, we change the selection direction to eDirPrevious, the last range
will be excluded by AutoPrepareFocusRange.

We need to set the range as being generated on the opposite side so that
AutoPrepareFocusRange could figure out the correct anchor focus range.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Part 1 only should fail on truck.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d698efc07955

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Part 1 and 2.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55c2cb041ba2

Comment 5

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0)
> Steps to reproduce:
> 1. Open about:config, enable "selectioncaret.enabled", and restart browser.

FYI: I did that, starting from a fresh profile, but I didn't get any
selection handles.  Nightly Firefox on Linux.

Comment 6

[Mats Palmgren (inactive)]

Sorry for breaking this feature.  I'd like to understand this code
a little better so I can avoid that in the future.

So, I gather that the selection-caret feature is using the regular
Selection and Range code, just with some special event handling and
tweaks on top, correct?

One of the tweaks is that you don't want to flip the selection around
when extending it in the opposite direction.  In other words, you
always want to treat a shift-click/drag-select to a point P as if
the Selection anchor->focus direction were towards P?  (even when
it's not)

So for example, for a Selection from B -> C (in that direction):
AAA...|BBB...CCC>
when the user "extends" that selection to A what you want is:
|AAA...BBB...CCC>
in the selection-caret case, rather than what we normally do:
<AAA...|BBB...CCC

(where | denotes the anchor and, < or > the focus with its direction
indicating the Selection direction)

Is that correct?

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #5)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0)
> > Steps to reproduce:
> > 1. Open about:config, enable "selectioncaret.enabled", and restart browser.
> 
> FYI: I did that, starting from a fresh profile, but I didn't get any
> selection handles.  Nightly Firefox on Linux.

I see the same issue from the nightly build, but I can always see the handle from my own debug build. Opened bug 1142853

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #6)
> Sorry for breaking this feature.  I'd like to understand this code
> a little better so I can avoid that in the future.
> 
> So, I gather that the selection-caret feature is using the regular
> Selection and Range code, just with some special event handling and
> tweaks on top, correct?

Yes. You are right.

> One of the tweaks is that you don't want to flip the selection around
> when extending it in the opposite direction.  In other words, you
> always want to treat a shift-click/drag-select to a point P as if
> the Selection anchor->focus direction were towards P?  (even when
> it's not)

Yes. On B2G, we always want to extend the selection to the point P as we move the caret around instead of flipping the selection. We did this by setting the current selection direction immediately to eDirPrevious while hitting the left selection-caret, and eDirPrevious while hitting the right selection-caret [1]. In this way, when dragging one caret to extend the selection, the anchor will always on other side with the other caret.

> 
> So for example, for a Selection from B -> C (in that direction):
> AAA...|BBB...CCC>
> when the user "extends" that selection to A what you want is:
> |AAA...BBB...CCC>
> in the selection-caret case, rather than what we normally do:
> <AAA...|BBB...CCC
> 
> (where | denotes the anchor and, < or > the focus with its direction
> indicating the Selection direction)
> 
> Is that correct?

The selection range part is correct, but the anchor and focus should be switched. When the user "extends" that selection to A, he could only do this by dragging the left caret since we enforce the left caret and right caret could never cross. Therefore, in selection-caret case, the selection should be:
<AAA...BBB...CCC|

Mats, if you want to know more, we have UX specs pdf attached in bug 921965 which should contain every use cases about text selection by selection-carets.

[1] http://hg.mozilla.org/mozilla-central/file/42afc7ef5ccb/layout/base/SelectionCarets.cpp#l214

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8575754 [details] [diff] [review]
Part 2 - Change the opposite range as generated when dragging caret.

Close, but not quite right I think.

I think you should make SetSelectionDirection() change the 'IsGenerated'
bit on the first and last ranges in the selection (when there's more
than 2).

In the eDirPrevious case, the last range should have the bit set and
first range should have it unset (vice versa for eDirNext).
You don't need to change other ranges, they should already have the
bit set.

In other words: all ranges in the Selection should have the bit set
except the range on the end side in the selection direction.

Comment 10

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #8)

OK, thanks for the explanation.  So it seems the selection-caret code
doesn't really care about the existing direction of the underlying
Selection since it calls SetSelectionDirection that sets it depending
on which handle was clicked on, right?

I looked at the design document in bug 921965 (interesting!).  It seems
dragging the handles is the only means of modifying the selection.
I didn't see any mention of keyboard selection (e.g. Shift+LeftArrow) or
Shift+click outside the current selected region.  Are those use cases not
handled at all in the selection-caret code?  (which is fine, I'm just
curious about how this code works)

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Change the opposite range as generated when dragging caret. (v2)

When dragging the second caret (eDirNext) over non-selectable elements,
the last range is not generated. Therefore when dragging the first
caret, we will change the selection direction to eDirPrevious, the last
range will be excluded by AutoPrepareFocusRange.

When changing the selection direction to eDirPrevious, we should set the
generated bit of the last range, and unset the bit of the first
range (vice versa for eDirNext). In this way, AutoPrepareFocusRange
could figure out the correct anchor focus range.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7617a5910248

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #10)
> OK, thanks for the explanation.  So it seems the selection-caret code
> doesn't really care about the existing direction of the underlying
> Selection since it calls SetSelectionDirection that sets it depending
> on which handle was clicked on, right?

You are right! 

> I looked at the design document in bug 921965 (interesting!).  It seems
> dragging the handles is the only means of modifying the selection.
> I didn't see any mention of keyboard selection (e.g. Shift+LeftArrow) or
> Shift+click outside the current selected region.  Are those use cases not
> handled at all in the selection-caret code?  (which is fine, I'm just
> curious about how this code works)

SelectionCarets does not handle keyboard selection at all since we only have touchscreen on B2G :-)
To initiate a selection, the user could long press to select a word. And then drag the caret handle to change the selection range.

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8577857 [details] [diff] [review]
Part 2 - Change the opposite range as generated when dragging caret. (v2)

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

After addressing comment 9, it turns out that part 2 (v2) does not pass the test case added by part 1.

::: layout/base/SelectionCarets.cpp
@@ +883,5 @@
> +      nsRange* firstRange = selection->GetRangeAt(0);
> +      nsRange* lastRange = selection->GetRangeAt(rangeCount - 1);
> +
> +      if (aDir == eDirPrevious) {
> +        firstRange->SetIsGenerated(false);

If we unset the generated bit of firstRange here, it will not be removed by AutoPrepareFocusRange since it was not generated nor being the old mAnchorFocusRange.

So, if we have to set the firstRange as generated, we need to call  the private Selection::setAnchorFocusRange(0) to set firstRange as the mAnchorFocusRange as well. In this way, the firstRange will be removed by AutoPrepareFocusRange.

Or could we just set lastRange as generated, and leave the generated bit of firstRange untouched?

Comment 14

[Mats Palmgren (inactive)]

Right, we need to fix up mAnchorFocusRange as well:
selection->setAnchorFocusRange(0); for eDirPrevious and
selection->setAnchorFocusRange(rangeCount - 1) for eDirNext.

setAnchorFocusRange is private though, so it probably makes sense to move
the contents of SetSelectionDirection to a method on Selection instead.

I debugged this a bit and there's an additional change you need.
There's a lingering non-null 'mMaintainRange' that makes us return
early from nsFrameSelection::HandleClick.  I'm not sure exactly where
that comes from, perhaps SelectionCarets::SelectWord() ?
We need to reset that, so that HandleClick works correctly for drag.
Here's what I did to make the test pass:

diff --git a/layout/base/SelectionCarets.cpp b/layout/base/SelectionCarets.cpp
--- a/layout/base/SelectionCarets.cpp
+++ b/layout/base/SelectionCarets.cpp
@@ -731,16 +731,17 @@ SelectionCarets::DragSelection(const nsP
   if (!ptFrame) {
     return nsEventStatus_eConsumeNoDefault;
   }
 
   nsRefPtr<nsFrameSelection> fs = GetFrameSelection();
   if (!fs) {
     return nsEventStatus_eConsumeNoDefault;
   }
+  fs->MaintainSelection();
 

I'm not sure if that's the best place for it, but it seems to work.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Fwiw, some debug stuff you might find useful

Here's some debug code I wrote while working on bug 1129078.
You can call "DumpRanges(selection)" and it will print out
the ranges and say which ones are "generated" and which one
is the mAnchorFocusRange (indicated with a '>' character in
the list).

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8577857 [details] [diff] [review]
Part 2 - Change the opposite range as generated when dragging caret. (v2)

Looks good so far, but see comment 14.

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Adjust ranges after changing selection direction. r=mats

When dragging the second caret (eDirNext) over non-selectable elements,
the last range is not generated. Therefore when dragging the first
caret, we will change the selection direction to eDirPrevious, the last
range will be excluded by AutoPrepareFocusRange.

We add a new function to adjust generated bit and anchor focus range
after changing selection direction.

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 3 - Clear maintain selection when dragging caret. r=mats

After double-clicking to select a word on browser, we cannot drag caret
to change the selection within a word. We should clear the maintained
selection.

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The mMaintainRange will be cleared by SelectionCarets::SelectWord() in [1], but it won't be cleared when double-clicking to select a word on browser. It's good to cleared the maintain range while dragging as you suggested in comment 14. Added patch part 3 to do that. Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/436686833af0/layout/base/SelectionCarets.cpp#l644

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8578471 [details] [diff] [review]
Part 2 - Adjust ranges after changing selection direction. r=mats

Perhaps it would be a good idea to let AdjustAnchorFocusForMultiRange
do the SetDirection call itself, if needed?  i.e. make it take the
direction as an argument and then add at the start of the method:

if (aDirection == mDirection {
  return;
}
SetDirection(aDirection);
...

Either way is fine though, r=mats

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8578472 [details] [diff] [review]
Part 3 - Clear maintain selection when dragging caret. r=mats

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #19)
> it won't be cleared when double-clicking to select a word on browser.

Ah, right.  Thanks for looking in to it.

Comment 22

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Adjust ranges after changing selection direction. (v2, carry r+)

Adapt Mats suggestion in Comment 20.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63bda71f7215

Comment 23

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Adjust ranges after changing selection direction. (v3, carry r+)

Add #include "nsDirection.h" in SelectionCaret.h to avoid future compile errors
due to unified build.

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Please help checkin Part 1 ~ 3. Thanks.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17a769dd16a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/44438a4eef3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5ef4601d3a

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/17a769dd16a1
https://hg.mozilla.org/mozilla-central/rev/44438a4eef3e
https://hg.mozilla.org/mozilla-central/rev/db5ef4601d3a