Regression: Selection anchor + focus swap when arrow keys after setBaseAndExtent

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Danilatos]

The following code (setting the selection base and extent to itself) should be a no-op:

      var selection = window.getSelection();
      selection.setBaseAndExtent(
          selection.anchorNode, selection.anchorOffset,
          selection.focusNode, selection.focusOffset);

However, if there is a ranged selection, and this code is executed, then holding shift and using the left or right arrow keys, after having run that code, causes the focus to switch to the side that would cause the selection always to expand, rather than preserving the current focus.

STEPS TO REPRODUCE:
1. Go to the demo URL
2. Click on the text in the contentEditable area
3. Hold down shift, and press the left and right arrow keys in different combinations.

EXPECTED BEHAVIOUR:
One end of the selection stays fixed. The other end moves under the command of the arrow keys.

OBSERVED BEHAVIOUR:
The selection continually expands in both directions


This regression appears somewhere between webkit nightlies r42162 and r49550 (Those were the two I had lying around, I haven't done a proper binary search).

[Daniel Danilatos]

Note, if you set dir=rtl on the block level element, the bug manifests itself differently: it is now impossible to do any kind of range selection. E.g. hold shift and repeatedly press the right arrow key, the selection will expand to the right and simultaneously keep contracting from the left.

[Julie Parent]

To narrow regression range slightly, this reproduces in r48037.

[Ojan Vafai]

Regression range is 45934:45980

[Ojan Vafai]

For those following along, this bug is essentially about being able to save a selection, mess with the DOM and restore the selection in a way that maintains platform appropriate directionality.

http://trac.webkit.org/changeset/45945 caused the regression. The problem here is that on the Mac the first time you hit shift+left/right decides which end of the selection is the anchor/focus. That state is stored in m_lastChangeWasHorizontalExtension, which is not exposed to JS in any way.

That this happens on all platforms is a bug. I have an unfinished patch that makes this only happen on the Mac. But still, for this to work on the Mac, we'd need to expose something to JS. Something like isDirectionalityFinal. On non-Mac platforms, that would always return true. Then, setBaseAndExtent should take an optional fifth argument that is the value of this boolean and it would set m_lastChangeWasHorizontalExtension accordingly.

Not sure who the arbiters of adding to DOMSelection.idl are. Hyatt, Mitz you have thoughts? It's a bit weird to me to expose platform-specific features of selections to the web, but I don't really see an alternative.

Is it possible to just refine the rules for when m_lastChangeWasHorizontalExtension is reset?

[Ojan Vafai]

setSelection (which is reached via the JS call to setBaseAndExtent) can't know that you're trying to restore the previous selection. In the cases that Wave cares about (Daniel is on the Wave team), the DOM might be different (e.g. might have extra spans), but to the user it looks the same.

We could say that setBaseAndExtent should not set m_lastChangeWasHorizontalExtension to false. That would work for Wave's use-case, but I can think of use cases where this would break. For example, a web app the pops up a modal dialog with a text box in it. When the dialog is closed, it will want to restore the previous selection, but edits in the dialog's text box might have messed with the m_lastChangeWasHorizontalExtension state.

In short, I'm not sure how setSelection could know what to do with m_lastChangeWasHorizontalExtension.

[Ojan Vafai]

I have a patch for this. I'm still open to refining the rules for when
m_lastChangeWasHorizontalExtension is reset. It's obviously preferable to not expose this to the web, but I don't see how that can be accomplished.

I'll post the patch for review, but I'm totally open to doing this differently if anyone has better idea.

Another possibility of course would be to not implement this crazy mac selection behavior, but I'll assume that's a dead end. :)

[Ojan Vafai]

Created attachment 48601 [details]
Patch

[Ojan Vafai]

Created attachment 51195 [details]
Patch

[Ojan Vafai]

Comment on attachment 51195 [details]
Patch

Here's the only way I can think of fixing this without exposing isDirectional to the web. It's not perfect, but it does avoid exposing more information to JS.

The advantage is it makes this test-case and the common case work. It breaks the following case:
1. Select text via the mouse.
2. Save the selection.
3. Do some stuff in your app that clears the selection.
4. Restore the selection.

After step 1, the selection is non-directional. After step 4, the selection is directional.

While it's not perfect, that's much better than the current state of affairs that leads to bugs like the ones in this testcase.

The only way to get full selection fidelity would be the first, now obsoleted, patch. I'm happy with either of these patches.

[Adam Barth]

Comment on attachment 51195 [details]
Patch

This seems like a failure of the API to me.  However, defaulting the selections to directional makes sense given that the name of the API implies directionality.  Ojan, we need to convince you to write a spec for all this stuff.

LayoutTests/editing/selection/5195166-1.html:6
 +    document.getElementById("console").innerHTML += str + "<br>";
Bad indent.

[Ojan Vafai]

Created attachment 60770 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 60770 [details]
Patch for landing

Rejecting patch 60770 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
was:
--------------------------
|Index: WebCore/WebCore.base.exp
|index 70f07735caf166fd754d101a32cada4f6a89bad2..4dd27b88ffc8f4ed2d65e1fdbceffb5e5cf36875 100644
|--- WebCore/WebCore.base.exp
|+++ WebCore/WebCore.base.exp
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file WebCore/editing/SelectionController.cpp
Hunk #1 succeeded at 107 (offset 1 line).
patching file WebCore/editing/SelectionController.h
patching file WebCore/page/EventHandler.cpp

Full output: http://webkit-commit-queue.appspot.com/results/3446011

[Darin Adler]

Comment on attachment 60770 [details]
Patch for landing

> -        m_frame->selection()->setSelection(newSelection, m_frame->selectionGranularity());
> +        m_frame->selection()->setSelection(newSelection, m_frame->selectionGranularity(), false);

This is a classic example of why we don't use boolean arguments when constants are passed. This patch would be considerably better if the argument used an enum with good names instead.

[Ojan Vafai]

(In reply to comment #14)
> (From update of attachment 60770 [details])
> > -        m_frame->selection()->setSelection(newSelection, m_frame->selectionGranularity());
> > +        m_frame->selection()->setSelection(newSelection, m_frame->selectionGranularity(), false);
> 
> This is a classic example of why we don't use boolean arguments when constants are passed. This patch would be considerably better if the argument used an enum with good names instead.

enum Directionality { DirectionalityNone, DirectionalityAware }; was the best I could come up with. Does that work?

This patch has changed enough since the r+ from Adam, that I think it's worth getting another review.

[Ojan Vafai]

Created attachment 60917 [details]
Patch

[Darin Adler]

Comment on attachment 60917 [details]
Patch

I’d rather see the enum at the global level rather than as a member. I don’t think there’s any real danger of conflict with other similar enums.

I think we could find a better name for the enum by talking about what it is and selecting words from that discussion. For example we could consider using verbs like “MakeNonDirectionalSelection” and “MakeDirectionalSelection”. The word “policy” in the name of the enum allows the enum values to be verbs instead of adjectives or nounds.

Fine as-is, though. r=me

[Ojan Vafai]

(In reply to comment #17)
> (From update of attachment 60917 [details])
> I’d rather see the enum at the global level rather than as a member. I don’t think there’s any real danger of conflict with other similar enums.
> 
> I think we could find a better name for the enum by talking about what it is and selecting words from that discussion. For example we could consider using verbs like “MakeNonDirectionalSelection” and “MakeDirectionalSelection”. The word “policy” in the name of the enum allows the enum values to be verbs instead of adjectives or nounds.
> 
> Fine as-is, though. r=me

Thanks. Both of those sound good to me. I'll change the enum to the following and commit:
enum DirectionalityPolicy { MakeNonDirectionalSelection, MakeDirectionalSelection };

[Ojan Vafai]

Committed r62816: <http://trac.webkit.org/changeset/62816>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62816 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #20)
> http://trac.webkit.org/changeset/62816 might have broken Qt Linux Release

pretty diff: http://build.webkit.org/results/Qt%20Linux%20Release/r62848%20%2814976%29/editing/selection/5195166-1-pretty-diff.html

[Ojan Vafai]

(In reply to comment #21)
> (In reply to comment #20)
> > http://trac.webkit.org/changeset/62816 might have broken Qt Linux Release
> 
> pretty diff: http://build.webkit.org/results/Qt%20Linux%20Release/r62848%20%2814976%29/editing/selection/5195166-1-pretty-diff.html

Antonio was looking into this, but he seems to have disappeared from #webkit. It looks to me like a bug in QTs implementation of double-click + drag to select text. Should I add it to the Skipped list for now?