1168891 - New Gecko selection carets cannot reverse their positions

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Jan Henning [:JanH]]

Attachment: visualisation of old and new behaviour

Steps to reproduce:
Activate the new selection carets by setting selectioncaret.enabled and touchcaret.enabled to true.
Pick a paragraph with a bit of text and select some text in the middle of it.
Take the second selection caret (end of selection) and try dragging it towards the beginning of the paragraph.

Results:
Dragging stops when you reach the first selection caret (beginning of selection).

Expected result:
With the old selection carets, it is possible to continue dragging the second caret right across the beginning of the selection. After releasing the caret, the two carets (beginning and end) will simply swap their positions. See also the attached visualisation.

Comment 1

[Mark Capella [:capella]]

This is normal behaviour with the new Carets.

Comment 2

[Mark Capella [:capella]]

cc: tylin for historical perspective? I haven't looked into the original b2g implementation detail here, but I've been imagining this is pretty in-grained. Is it feasible / worth the time to guess-timate an effort to retro-fit the original Fennec Handle behaviour?

Comment 3

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

This behavior is by design as described in page 12 "Carets can’t cross each other" in [1], so the minimum selection is one character. This in in-grained in the caret dragging logic. From the UX perspective, the user won't accidentally reverse the selection if he/she drags the end caret upward as in page 12 "Dragging Upward". If possible, I would prefer not to maintain two different behaviors on two platforms.

[1] https://bug921965.bugzilla.mozilla.org/attachment.cgi?id=8548759

Comment 4

[Jan Henning [:JanH]]

Hmm, I guess on the one hand I can understand the desire for a consistent behaviour across all of Mozilla's products. On the other hand, I'd like to argue that there's also the consistency across the Android platform to be considered.

First, I've done a non-representative survey across the apps installed on my phone. I have to admit the picture there is rather inconsistent - some apps allow crossing the text selection handles, others don't, without a really clear pattern discernible.
Next, I've focussed specifically on browsers, so I installed a number of alternative browsers. I've tested the JB 4.1.2 stock browser, Chrome, UC Browser, Opera, Opera Mini, Maxthon, Boat Browser and Dolphin. All of them allow the selection handles to cross each other.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Android platform behavior and convenience should be considered here.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> Android platform behavior and convenience should be considered here.

Chrome for Android does flip the handles. We think that we want the Gecko handles to flip as well, but it won't block shipping the Gecko handles, hence tracking +.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)
> Android platform behavior and convenience should be considered here.

I absolutely agree here. As an Android user it feels absolutely odd.

Comment 9

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

I'm working on this.

Comment 10

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

Attachment: MozReview Request: Bug 1168891 Part 1 - Refine two functions related to caret positioning.

FindFirstNodeWithFrame() and CompareRangeWithContentOffset() share a lot
duplication code. I refactor and rename the two functions to improve the
readability.

Review commit: https://reviewboard.mozilla.org/r/44741/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44741/

Comment 11

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

Attachment: MozReview Request: Bug 1168891 Part 2 - Allow one caret to be dragged across the other caret.

This is the convention of the Android platform.

Review commit: https://reviewboard.mozilla.org/r/44743/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44743/

Comment 12

[Mats Palmgren (inactive)]

I don't see any reason to make this preffable.  If we think flipping the carets
is the best user experience then that should be the default on all platforms.
Having to maintain code and tests for both behaviors has a cost.

Comment 13

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

(In reply to Mats Palmgren (:mats) from comment #12)
> I don't see any reason to make this preffable.  If we think flipping the carets 
> is the best user experience then that should be the default on all platforms.
> Having to maintain code and tests for both behaviors has a cost.

I had the same feeling for not to maintain two behaviors in comment 3.  However, the current non-swappable carets are required per B2G text selection spec, and AFAIK iOS carets cannot swap positions, too.  If we want to match the convention on different platforms, the cost might be unavoidable. 

It would be hard if we have to choose one convention over the other this moment especially AccessibleCaret might be needed on Windows in bug 1195722 (I don't know the convention yet) or on iOS (if we can get gecko into iOS).  We should have a separate bug to discuss removing one behavior if it's proven that the maintenance cost is too much, or the particular behavior isn't used anymore.

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 8738891 [details]
MozReview Request: Bug 1168891 Part 1 - Refine two functions related to caret positioning.

https://reviewboard.mozilla.org/r/44741/#review41877

::: layout/base/AccessibleCaretManager.cpp:887
(Diff revision 1)
> -  }
> +  int32_t nodeOffset = 0;
> +  CaretAssociationHint hint;
>  
> -  uint32_t rangeCount = selection->RangeCount();
> -  if (rangeCount <= 0) {
> -    return nullptr;
> +  RefPtr<Selection> selection = GetSelection();
> +  bool findInFirstRangeStart = aDirection == eDirNext;
> +

I think we still need this check (that we have at least one range).

::: layout/base/AccessibleCaretManager.cpp:966
(Diff revision 1)
> -  if (!theFrame) {
> -    return false;
> -  }
>  
> -  // Move one character forward/backward from point and get offset
> -  nsPeekOffsetStruct pos(eSelectCluster,
> +  // Move one character in the direction of dir from the node offset and get
> +  // the new offset.

I think calling it "new offset" is confusing since it can be misread as "the active caret's new position" (which is what aOffsets is if I'm not mistaken).

|pos| is one character forward/backward from the inactive caret, right?

So this comment would be clearer as something like:
"Move one character (in the direction of dir) from the inactive caret's position.  This is the limit for the active caret's new position."

::: layout/base/AccessibleCaretManager.cpp:975
(Diff revision 1)
>    if (NS_FAILED(rv)) {
>      pos.mResultContent = content;
>      pos.mContentOffset = nodeOffset;
>    }
>  
> -  // Compare with current point
> +  // Compare the new offset with the given aOffsets.

This comment would be clearer as:
"Compare the active caret's new position (aOffsets) to the limit."

::: layout/base/AccessibleCaretManager.cpp:976
(Diff revision 1)
>      pos.mResultContent = content;
>      pos.mContentOffset = nodeOffset;
>    }
>  
> -  // Compare with current point
> -  int32_t result = nsContentUtils::ComparePoints(aOffsets.content,
> +  // Compare the new offset with the given aOffsets.
> +  int32_t cmp_pos =

nit: "cmp_pos" doesn't follow naming conventions for local variables (which is camelCase).

And looking ahead to the next patch, you use "cmp_other_caret" for basically the same ComparePoints call.

It would be nice to have a description of the aOffsets param to RestrictCaretDraggingOffset in the header file.  As far as I can tell it's the new position of the caret that is being dragged(?) and the local variables node/nodeOffset is the position of the other (inactive) caret(?)

::: layout/base/AccessibleCaretManager.cpp:981
(Diff revision 1)
> -  int32_t result = nsContentUtils::ComparePoints(aOffsets.content,
> -                                                 aOffsets.StartOffset(),
> -                                                 pos.mResultContent,
> -                                                 pos.mContentOffset);
> -  if ((mActiveCaret == mFirstCaret.get() && result == 1) ||
> -      (mActiveCaret == mSecondCaret.get() && result == -1)) {
> +  int32_t cmp_pos =
> +    nsContentUtils::ComparePoints(aOffsets.content, aOffsets.StartOffset(),
> +                                  pos.mResultContent, pos.mContentOffset);
> +  if ((mActiveCaret == mFirstCaret.get() && cmp_pos == 1) ||
> +      (mActiveCaret == mSecondCaret.get() && cmp_pos == -1)) {
> +    // aOffset is across the new offset. Set it to the new offset.

This comment would be clearer as:
"The active caret's position is past the limit, which we don't allow here.  So set it to the limit, resulting in one character being selected."

Perhaps the code would be clearer if we renamed |pos| to |limit| ?
and |cmp_pos| to |cmpToLimit| ?

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8738892 [details]
MozReview Request: Bug 1168891 Part 2 - Allow one caret to be dragged across the other caret.

https://reviewboard.mozilla.org/r/44743/#review41881

::: layout/base/AccessibleCaretManager.h:304
(Diff revision 1)
>    // UI interactions. Optionally, we can try to maintain the active UI, keeping
>    // carets and ActionBar available.
>    static bool sCaretsScriptUpdates;
>  
> +  // Preference to allow one caret to be dragged across the other caret without
> +  // any limitation. By default, one caret cannot be dragged across the other

Please update the comment so that it's neutral on what is the default.  For example by replacing the second sentence with "When set to false, ..."

::: layout/base/AccessibleCaretManager.cpp:81
(Diff revision 1)
>  /*static*/ bool
>  AccessibleCaretManager::sCaretsAlwaysTilt = false;
>  /*static*/ bool
>  AccessibleCaretManager::sCaretsScriptUpdates = false;
>  /*static*/ bool
> +AccessibleCaretManager::sCaretsAllowDraggingAcrossOtherCaret = false;

Please initialize it to true here. (see last comment below)

::: layout/base/AccessibleCaretManager.cpp:109
(Diff revision 1)
>                                   "layout.accessiblecaret.extendedvisibility");
>      Preferences::AddBoolVarCache(&sCaretsAlwaysTilt,
>                                   "layout.accessiblecaret.always_tilt");
>      Preferences::AddBoolVarCache(&sCaretsScriptUpdates,
>        "layout.accessiblecaret.allow_script_change_updates");
> +    Preferences::AddBoolVarCache(&sCaretsAllowDraggingAcrossOtherCaret,

Please add a third arg, true, here.  (see last comment below)

::: layout/base/AccessibleCaretManager.cpp:970
(Diff revision 1)
>    }
>  
>    nsCOMPtr<nsIContent> content = do_QueryInterface(node);
>  
> +  if (sCaretsAllowDraggingAcrossOtherCaret) {
> +    // Compare the position of the other caret with the given aOffsets.

The comment would slightly clearer if you replace "other" with "inactive", or "other (inactive)".

::: layout/base/AccessibleCaretManager.cpp:971
(Diff revision 1)
>  
>    nsCOMPtr<nsIContent> content = do_QueryInterface(node);
>  
> +  if (sCaretsAllowDraggingAcrossOtherCaret) {
> +    // Compare the position of the other caret with the given aOffsets.
> +    int32_t cmp_other_caret =

Please rename |cmp_other_caret| to |cmpToOtherCaretPos| or |cmpToInactiveCaretPos| or something like that.

::: layout/base/AccessibleCaretManager.cpp:975
(Diff revision 1)
> +    // Compare the position of the other caret with the given aOffsets.
> +    int32_t cmp_other_caret =
> +      nsContentUtils::ComparePoints(aOffsets.content, aOffsets.StartOffset(),
> +                                    content, nodeOffset);
> +    if (mActiveCaret == mFirstCaret.get()) {
> +      if (cmp_other_caret == 0) {

Wouldn't it be simpler to use a switch, like so:

switch (cmp_other_caret) {
  case 0:
    // aOffset is the same as the offset of the other caret...
    return false;
  case 1:
    if (mActiveCaret == mFirstCaret.get()) {
      ...
    }
    break;
  case -1:
    if (mActiveCaret == mSecondCaret.get()) {
      ...
    }
    break;
}

::: layout/base/AccessibleCaretManager.cpp:980
(Diff revision 1)
> +      if (cmp_other_caret == 0) {
> +        // The aOffset is the same as the offset of the second caret. Return
> +        // false so that the selection range is not collapsed by the caller.
> +        return false;
> +      } else if (cmp_other_caret == 1) {
> +        // aOffset is moved bypass the second caret. After making change to the

Please change "is moved bypass" to "was moved across".

::: modules/libpref/init/all.js:5019
(Diff revision 1)
>  // AccessibleCarets and close UI interaction by default.
>  pref("layout.accessiblecaret.allow_script_change_updates", false);
>  
> +// By default, one caret cannot be dragged across the other one, i.e. the
> +// minimum selection range will be one character.
> +pref("layout.accessiblecaret.allow_dragging_across_other_caret", false);

I think this should be true by default, and then we should set it to false in case some platform needs that in the future.

(When 'layout.accessiblecaret.enabled' is enabled in a Firefox desktop build, we should allow dragging across the other caret by default since that's how native selection works on all desktop platforms.)

Comment 16

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #13)
> non-swappable carets are required per B2G text selection spec

Well, since we now think that swappable carets is a better
user experience shouldn't we update that document?

> If we want to match the convention on different platforms,
> the cost might be unavoidable.

Sure, but is selection behavior merely built-in convention?
or is it more like a feature of the app where apps can make
improvements?  To me, it seems a pity to deny users a better
UX just because of conventions.  But, that's for UX people
to decide I guess...

Comment 17

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

Comment on attachment 8738891 [details]
MozReview Request: Bug 1168891 Part 1 - Refine two functions related to caret positioning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44741/diff/1-2/

Comment 18

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

Comment on attachment 8738892 [details]
MozReview Request: Bug 1168891 Part 2 - Allow one caret to be dragged across the other caret.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44743/diff/1-2/

Comment 19

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

https://reviewboard.mozilla.org/r/44741/#review41877

> I think we still need this check (that we have at least one range).

The above "GetCaretMode() == CaretMode::Selection" should guarentee we have at least one range.

> I think calling it "new offset" is confusing since it can be misread as "the active caret's new position" (which is what aOffsets is if I'm not mistaken).
> 
> |pos| is one character forward/backward from the inactive caret, right?
> 
> So this comment would be clearer as something like:
> "Move one character (in the direction of dir) from the inactive caret's position.  This is the limit for the active caret's new position."

You're right. |pos| will be used to get the limit for the active caret's new position. Your comment is better.

> This comment would be clearer as:
> "Compare the active caret's new position (aOffsets) to the limit."

Fixed.

> nit: "cmp_pos" doesn't follow naming conventions for local variables (which is camelCase).
> 
> And looking ahead to the next patch, you use "cmp_other_caret" for basically the same ComparePoints call.
> 
> It would be nice to have a description of the aOffsets param to RestrictCaretDraggingOffset in the header file.  As far as I can tell it's the new position of the caret that is being dragged(?) and the local variables node/nodeOffset is the position of the other (inactive) caret(?)

Aha! I must rename the variable cmp_pos after writing the test in Python ...

Yes. aOffsets is the new position of the active caret where the local variables node/nodeOffset is the position of the inactive caret.

> This comment would be clearer as:
> "The active caret's position is past the limit, which we don't allow here.  So set it to the limit, resulting in one character being selected."
> 
> Perhaps the code would be clearer if we renamed |pos| to |limit| ?
> and |cmp_pos| to |cmpToLimit| ?

Agree. Rename in the next patch.

Comment 20

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

https://reviewboard.mozilla.org/r/44743/#review41881

> Wouldn't it be simpler to use a switch, like so:
> 
> switch (cmp_other_caret) {
>   case 0:
>     // aOffset is the same as the offset of the other caret...
>     return false;
>   case 1:
>     if (mActiveCaret == mFirstCaret.get()) {
>       ...
>     }
>     break;
>   case -1:
>     if (mActiveCaret == mSecondCaret.get()) {
>       ...
>     }
>     break;
> }

Yes. I would be simpler to use a switch! BTW, in case 0: I feel it would clearer to set the aOffsets to |limit| then `return false` to make the caller to skip `HandleClick`. See my patch set 2.

> I think this should be true by default, and then we should set it to false in case some platform needs that in the future.
> 
> (When 'layout.accessiblecaret.enabled' is enabled in a Firefox desktop build, we should allow dragging across the other caret by default since that's how native selection works on all desktop platforms.)

You convinced me. Allowing dragging caret across other caret is closed to the built-in selection behavior on all desktop platform. I'll make it true by default.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8738891 [details]
MozReview Request: Bug 1168891 Part 1 - Refine two functions related to caret positioning.

https://reviewboard.mozilla.org/r/44741/#review42139

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 8738892 [details]
MozReview Request: Bug 1168891 Part 2 - Allow one caret to be dragged across the other caret.

https://reviewboard.mozilla.org/r/44743/#review42141

Looks great!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fed9d8896b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6915c6f22667

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fed9d8896b2
https://hg.mozilla.org/mozilla-central/rev/6915c6f22667

Comment 25

[Jan Henning [:JanH]]

Working beautifully on today's Nightly, thanks very much.