Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=695317
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests --gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests --gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement

Review-Url: https://codereview.chromium.org/2720593005
Cr-Commit-Position: refs/heads/master@{#454236}
Committed: https://chromium.googlesource.com/chromium/src/+/2211504ac2b999a125b2215ce7f6be9e50878fea

[yosin_UTC9, 2017-02-28 11:12:47]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 11:13:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 12:19:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 12:19:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yosin_UTC9, 2017-02-28 14:07:25]

Description was changed from

==========
2017-02-28T20:10:50

2017-02-28T20:07:08

BUG=
==========

to

==========
2017-02-28T20:10:50

2017-02-28T20:07:08

BUG=693475
==========

[yosin_UTC9, 2017-02-28 17:22:43]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 17:23:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-02-28 17:26:21]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[yosin_UTC9, 2017-02-28 17:26:32]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-28 17:26:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 19:37:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 19:37:35]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-01 01:48:42]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 01:49:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 03:55:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 03:55:37]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-02 05:26:19]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 05:27:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-03-02 05:31:14]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[yosin_UTC9, 2017-03-02 05:31:37]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-02 05:33:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-03-02 06:46:31]

Description was changed from

==========
2017-02-28T20:10:50

2017-02-28T20:07:08

BUG=693475
==========

to

==========
Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=693475
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests
--gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests
--gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement
==========

[yosin_UTC9, 2017-03-02 06:46:32]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-03-02 06:46:39]

PTAL

[yoichio, 2017-03-02 07:31:06]

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:298: bool
isUserSelectContain(const Node& node) {
user-select:contain intends to let user selection not go
 "out" of a element.

If you want to let selection skip over some element,
 could you rename more better one?
Such like isUserSelectShouldSkipOver ?

[yosin_UTC9, 2017-03-02 07:47:17]

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:298: bool
isUserSelectContain(const Node& node) {
On 2017/03/02 at 07:31:06, yoichio wrote:
> user-select:contain intends to let user selection not go
>  "out" of a element.
> 
> If you want to let selection skip over some element,
>  could you rename more better one?
> Such like isUserSelectShouldSkipOver ?

No this function is named to detect specified node has "user-select:content"
rather than abstract of predicate for skipping node of |PositionIterator|.

It is too early to make abstraction of iteration for selection since we've not
yet have concrete idea yet.

BTW, for PositionIterator, we also want to skip contents for elements having
"user-select:none" and "user-select:all".
Handling of them should be in another patches.

[commit-bot: I haz the power, 2017-03-02 08:17:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 08:17:11]

Dry run: This issue passed the CQ dry run.

[yoichio, 2017-03-02 08:49:03]

On 2017/03/02 07:47:17, yosin_UTC9 wrote:
>
https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):
> 
>
https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/EditingUtilities.cpp:298: bool
> isUserSelectContain(const Node& node) {
> On 2017/03/02 at 07:31:06, yoichio wrote:
> > user-select:contain intends to let user selection not go
> >  "out" of a element.
> > 
> > If you want to let selection skip over some element,
> >  could you rename more better one?
> > Such like isUserSelectShouldSkipOver ?
> 
> No this function is named to detect specified node has "user-select:content"
> rather than abstract of predicate for skipping node of |PositionIterator|.
> 
> It is too early to make abstraction of iteration for selection since we've not
> yet have concrete idea yet.
> 
> BTW, for PositionIterator, we also want to skip contents for elements having
> "user-select:none" and "user-select:all".
> Handling of them should be in another patches.
I misunderstood. The spec specifies also prohibiting to go into a element.

BTW, could you add assert_selection of selectall test?

[yoichio, 2017-03-02 08:49:11]

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/PositionIterator.cpp (right):

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/PositionIterator.cpp:187:
isUserSelectContain(*m_anchorNode)) &&
This must be |!shouldTraverseChildren(*m_anchorNode)|.

[yosin_UTC9, 2017-03-02 09:16:54]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 09:17:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-03-02 09:17:17]

PTAL

Updated.

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/PositionIterator.cpp (right):

https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/PositionIterator.cpp:187:
isUserSelectContain(*m_anchorNode)) &&
On 2017/03/02 at 08:49:11, yoichio wrote:
> This must be |!shouldTraverseChildren(*m_anchorNode)|.

Good catch!
Done.

[yosin_UTC9, 2017-03-02 09:29:31]

Description was changed from

==========
Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=693475
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests
--gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests
--gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement
==========

to

==========
Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=695317
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests
--gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests
--gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement
==========

[yosin_UTC9, 2017-03-02 09:49:03]

On 2017/03/02 at 08:49:03, yoichio wrote:
> On 2017/03/02 07:47:17, yosin_UTC9 wrote:
> >
https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):
> > 
> >
https://codereview.chromium.org/2720593005/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/EditingUtilities.cpp:298: bool
> > isUserSelectContain(const Node& node) {
> > On 2017/03/02 at 07:31:06, yoichio wrote:
> > > user-select:contain intends to let user selection not go
> > >  "out" of a element.
> > > 
> > > If you want to let selection skip over some element,
> > >  could you rename more better one?
> > > Such like isUserSelectShouldSkipOver ?
> > 
> > No this function is named to detect specified node has "user-select:content"
> > rather than abstract of predicate for skipping node of |PositionIterator|.
> > 
> > It is too early to make abstraction of iteration for selection since we've
not
> > yet have concrete idea yet.
> > 
> > BTW, for PositionIterator, we also want to skip contents for elements having
> > "user-select:none" and "user-select:all".
> > Handling of them should be in another patches.
> I misunderstood. The spec specifies also prohibiting to go into a element.
> 
> BTW, could you add assert_selection of selectall test?

I'll add FrameSelectionTest since the issue is VisibleSelecitonInFlatTree.
VisibleSeleciton(in DOM tre) is fine.

Canary succeeds 
assert_selection('<input>foo', 'selectAll', '^<input>foo|')

[yosin_UTC9, 2017-03-02 10:05:52]

The CQ bit was checked by yosin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 10:06:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-03-02 10:06:47]

PTAL

[yoichio, 2017-03-02 10:33:10]

lgtm

[commit-bot: I haz the power, 2017-03-02 11:37:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 11:37:46]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-02 12:22:54]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-03-02 12:23:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 12:28:19]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488457374098950,
"parent_rev": "23972357b3fb5591905c94528ee39ae1d1351f34", "commit_rev":
"2211504ac2b999a125b2215ce7f6be9e50878fea"}

[commit-bot: I haz the power, 2017-03-02 12:28:57]

Description was changed from

==========
Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=695317
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests
--gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests
--gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement
==========

to

==========
Make PositionIterator to skip contents of INPUT/SELECT/TEXTAREA

This patch makes |PositionIterator| to skip child nodes of INPUT, SELECT and
TEXTAREA and adopt |mostBackwardCaretPosition()|[1] to pass correct value to
|PositionIterator| to make |canonicalizePositionOf()| not to return a position
inside user agent shadow tree, e.g. a position of inner editor for INPUT to
allow "SelectAll" command works correctly documents which start with INPUT,
SELECT, and TEXTAREA element.

Before this patch, if document starts with INPUT, "SelectAll" command selects
contents in inner editor because flat tree version of |PositionIterator|
returns such position for a position before INPUT. Then |VisibleSelection|
shrink selection into inner editor of INPUT to avoid crossing editing boundary.

This patch uses "user-select:contain"[2] idea for skipping contents of INPUT,
SELECT and TEXTAREA, to align with the spec[2] rather than using internal
term "selection boundary".

[1] "empty-document-stylewithcss.html" is failed if we don't change
|mostBackwardCaretPosition()|
[2] https://drafts.csswg.org/css-ui-4/#user-interaction

BUG=695317
TEST=webkit_unit_tests --gtest_filter=PositionIteratorTest.*
TEST=webkit_unit_tests
--gtest_filter=VisibleSelectionTest.SelectAllWithInputElement
TEST=webkit_unit_tests
--gtest_filter=VisibleUnitsTest.canonicalPositionOfWithInputElement

Review-Url: https://codereview.chromium.org/2720593005
Cr-Commit-Position: refs/heads/master@{#454236}
Committed:
https://chromium.googlesource.com/chromium/src/+/2211504ac2b999a125b2215ce7f6...
==========

[commit-bot: I haz the power, 2017-03-02 12:28:58]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2211504ac2b999a125b2215ce7f6...