Allow selection to skip over contenteditable

Allow selection to skip over contenteditable

BUG=264539
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157492

[dmazzoni, 2013-07-30 17:00:18]

Ping? I know this code can be gnarly, just wondering if
one of you has started to take a look before I forget
what I wrote. Thanks!

[kenjibaheux, 2013-08-30 20:49:20]

On 2013/07/30 17:00:18, Dominic Mazzoni wrote:
> Ping? I know this code can be gnarly, just wondering if
> one of you has started to take a look before I forget
> what I wrote. Thanks!

Since I saw contenteditable stuff, I took the liberty of adding yosin@ who has
been looking at this area.
Yosin: could you take a look? we need these to enable caret browsing which is a
highly requested a11y feature.

Thanks.

[yosin_UTC9, 2013-08-31 01:00:47]

How about change Position::isCandidate() to check caret browsing mode and
returns true as editable? 

If so, selection motion in caret browsing is as same as in editiable.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-contenteditable.html (right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:14: window.before =
document.getElementById("before");
nit: var before = ...

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:14: window.before =
document.getElementById("before");
nit: var before = ...

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:15: window.after =
document.getElementById("after");
nit: var after =

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:20:
shouldBe("window.getSelection().focusNode == after", "true");
You can write
shouldBe("sel.focusNode", "after")

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:24:
shouldBe("window.getSelection().focusNode.parentElement == before", "true");
You can write
shouldBe("sel.focusNode.paretElement", "before")

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html
(right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:16:
window.before = document.getElementById("before");
Just "var before = ..." is enough.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:22:
shouldBe("window.getSelection().focusNode == after", "true");
You can write:
shouldBe("window.getSelection().focusNode", "after");

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:27:
shouldBe("window.getSelection().focusNode.parentElement == before", "true");
You can write:
shouldBe("window.getSelection().focusNode", "after");

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:627: pos =
pos.next(CanSkipOverEditingBoundary);
Should we pass CanSkipOverEditingBoundary for other granularity, especially for
line?

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:629: pos =
pos.previous(CanSkipOverEditingBoundary);
What happen if caret is at the end of text field?

<input>
  <div> <!-- shadow DOM tree, "|" denotes caret -->
    foo|
  </div>
</input>

Is caret still in text field?

I think we don't have layout test to check this.

[dmazzoni, 2013-09-04 07:06:58]

Thanks. The cursor doesn't leave a text field now because the text field itself
enforces that the cursor / selection must be entirely inside. I'll add a test
for that, though.

[dmazzoni, 2013-09-05 07:57:29]

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-contenteditable.html (right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:14: window.before =
document.getElementById("before");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> nit: var before = ...

Then I can't reference it in shouldBe, unless I'm misunderstanding.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:20:
shouldBe("window.getSelection().focusNode == after", "true");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> You can write
> shouldBe("sel.focusNode", "after")

This doesn't work unless "sel" is a global.

As far as I know I can do it one of two ways:

1. Make "before", "after", and "sel" into globals so that shouldBe can evaluate
them
2. Use complete specifications inside shouldBe, for example:

    shouldBe("window.getSelection().focusNode == 
document.getElementById('after')", "true")

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:627: pos =
pos.next(CanSkipOverEditingBoundary);
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> Should we pass CanSkipOverEditingBoundary for other granularity, especially
for
> line?

Not needed. I updated the test to show that word and line work correctly too.

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:629: pos =
pos.previous(CanSkipOverEditingBoundary);
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> What happen if caret is at the end of text field?
> 
> <input>
>   <div> <!-- shadow DOM tree, "|" denotes caret -->
>     foo|
>   </div>
> </input>
> 
> Is caret still in text field?

Yes.

> I think we don't have layout test to check this.

How would you suggest testing this? When I set the selection within a text
field, window.getSelection().focusNode is not set to the textarea.

[yosin_UTC9, 2013-09-05 08:42:47]

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-contenteditable.html (right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:14: window.before =
document.getElementById("before");
On 2013/09/05 07:57:29, Dominic Mazzoni wrote:
> On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> > nit: var before = ...
> 
> Then I can't reference it in shouldBe, unless I'm misunderstanding.

On my desktop, below script works:
<div id="foo">x</div>
<script>
var foo = document.getElementById('foo');
shouldBeEqualToString('foo.textContent', 'x');
</script>

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:20:
shouldBe("window.getSelection().focusNode == after", "true");
On 2013/09/05 07:57:29, Dominic Mazzoni wrote:
> On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> > You can write
> > shouldBe("sel.focusNode", "after")
> 
> This doesn't work unless "sel" is a global.
> 
> As far as I know I can do it one of two ways:
> 
> 1. Make "before", "after", and "sel" into globals so that shouldBe can
evaluate
> them

eval() can see them.

Here is working sample:
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/editing/deleting/delet...

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:627: pos =
pos.next(CanSkipOverEditingBoundary);
On 2013/09/05 07:57:29, Dominic Mazzoni wrote:
> On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> > Should we pass CanSkipOverEditingBoundary for other granularity, especially
> for
> > line?
> 
> Not needed. I updated the test to show that word and line work correctly too.
Will be them in patch #3? I'm not sure why Line and Word granularity works w/o
CanSkipOverEditingBoundary.

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:629: pos =
pos.previous(CanSkipOverEditingBoundary);
On 2013/09/05 07:57:29, Dominic Mazzoni wrote:
> On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> > What happen if caret is at the end of text field?
> > 
> > <input>
> >   <div> <!-- shadow DOM tree, "|" denotes caret -->
> >     foo|
> >   </div>
> > </input>
> > 
> > Is caret still in text field?
> 
> Yes.
> 
> > I think we don't have layout test to check this.
> 
> How would you suggest testing this? When I set the selection within a text
> field, window.getSelection().focusNode is not set to the textarea.

We want to have internals.getSelection() which returns original values rather
than normalized values. window.getSelection() returns normalized values for
hiding shadow DOM nodes.

[dmazzoni, 2013-09-05 17:17:21]

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-contenteditable.html (right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:14: window.before =
document.getElementById("before");
On 2013/09/05 08:42:48, Yoshifumi Inoue wrote:
> On 2013/09/05 07:57:29, Dominic Mazzoni wrote:
> > On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> > > nit: var before = ...
> > 
> > Then I can't reference it in shouldBe, unless I'm misunderstanding.
> 
> On my desktop, below script works:
> <div id="foo">x</div>
> <script>
> var foo = document.getElementById('foo');
> shouldBeEqualToString('foo.textContent', 'x');
> </script>

You're right. I realized that the problem is only when shouldBe is called inside
a function, then it can't access variables that are local to that function. I
originally had a function when I wrote this, but since there isn't one anymore,
that isn't needed.

Fixed.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-contenteditable.html:24:
shouldBe("window.getSelection().focusNode.parentElement == before", "true");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> You can write
> shouldBe("sel.focusNode.paretElement", "before")

Done.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
File LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html
(right):

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:16:
window.before = document.getElementById("before");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> Just "var before = ..." is enough.

Done.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:22:
shouldBe("window.getSelection().focusNode == after", "true");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> You can write:
> shouldBe("window.getSelection().focusNode", "after");

Done.

https://codereview.chromium.org/20572005/diff/5001/LayoutTests/editing/select...
LayoutTests/editing/selection/skip-over-uneditable-in-contenteditable.html:27:
shouldBe("window.getSelection().focusNode.parentElement == before", "true");
On 2013/08/31 01:00:47, Yoshifumi Inoue wrote:
> You can write:
> shouldBe("window.getSelection().focusNode", "after");

Done.

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/20572005/diff/5001/Source/core/editing/FrameS...
Source/core/editing/FrameSelection.cpp:629: pos =
pos.previous(CanSkipOverEditingBoundary);
On 2013/09/05 08:42:48, Yoshifumi Inoue wrote:
> We want to have internals.getSelection() which returns original values rather
> than normalized values. window.getSelection() returns normalized values for
> hiding shadow DOM nodes.

I think I figured out a solution using
internals.youngestShadowRoot(...).getSelection() - take a look.

[yosin_UTC9, 2013-09-06 01:31:14]

internals.youngestShadowRoot(...).getSelection() Rocks!

https://codereview.chromium.org/20572005/diff/17001/Source/core/editing/Visib...
File Source/core/editing/VisiblePosition.cpp (right):

https://codereview.chromium.org/20572005/diff/17001/Source/core/editing/Visib...
Source/core/editing/VisiblePosition.cpp:82: }
Please fix compilation error, visibleposition.cpp(82) : warning C4715:
'WebCore::VisiblePosition::next' : not all control paths return a value.

https://codereview.chromium.org/20572005/diff/17001/Source/core/editing/Visib...
Source/core/editing/VisiblePosition.cpp:112: }
Please fix compilation error: visibleposition.cpp(113) : warning C4715:
'WebCore::VisiblePosition::previous' : not all control paths return a value

[dmazzoni, 2013-09-06 07:29:25]

https://codereview.chromium.org/20572005/diff/17001/Source/core/editing/Visib...
File Source/core/editing/VisiblePosition.cpp (right):

https://codereview.chromium.org/20572005/diff/17001/Source/core/editing/Visib...
Source/core/editing/VisiblePosition.cpp:112: }
On 2013/09/06 01:31:15, Yoshifumi Inoue wrote:
> Please fix compilation error: visibleposition.cpp(113) : warning C4715:
> 'WebCore::VisiblePosition::previous' : not all control paths return a value

Done, added assert_not_reached and same below.

[dmazzoni, 2013-09-09 22:09:53]

Ping, how does this look now?

[eseidel, 2013-09-09 22:12:33]

lgtm

Looks reasonable to me.  I'm very glad to see yosin reviewing since he's by far
the most active in editing code at the moment.

[dmazzoni, 2013-09-09 23:07:16]

Thanks! @yosin, any additional thoughts?

[yosin_UTC9, 2013-09-10 01:13:31]

On 2013/09/09 23:07:16, Dominic Mazzoni wrote:
> Thanks! @yosin, any additional thoughts?
ACK
Although, term CanSkipOverEditingBoundary is confusing.

[commit-bot: I haz the power, 2013-09-10 02:27:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/20572005/29001

[commit-bot: I haz the power, 2013-09-10 04:09:06]

Change committed as 157492