Selection API: Mutating a Range object after adding it to Selection should update Selection attribu…

Selection API: Mutating a Range object after adding it to Selection should update Selection attributes.

This CL fixes 182 sub-tests in external/wpt/selection/addRange-NN.html, however
it breaks 19 sub-tests.  This is due to the unpredictability of the current
Selection.addRange() implementation.

BUG=490206
Review-Url: https://codereview.chromium.org/2687273002
Cr-Commit-Position: refs/heads/master@{#449920}
Committed: https://chromium.googlesource.com/chromium/src/+/0cc18c1b352bd7a0e8107652399c823f5bb81196

[tkent, 2017-02-10 06:33:43]

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

[commit-bot: I haz the power, 2017-02-10 06:34:09]

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

[tkent, 2017-02-10 07:59:17]

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

[commit-bot: I haz the power, 2017-02-10 07:59:35]

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

[tkent, 2017-02-10 09:07:46]

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

[tkent, 2017-02-10 09:07:47]

yoichio@, yosin@, would you review this please?

Note that addRange-NN-expected.txt changes are ignorable.  I'll explain offline.

[commit-bot: I haz the power, 2017-02-10 09:30:49]

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

[commit-bot: I haz the power, 2017-02-10 09:30:50]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-10 10:26:28]

Could you add difference of number of PASS and FAIL for explaining improvement
from this patch?
For example:
  PASS +20
  FAIL +3

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/selection/selection-update-range-after-addrange.html
(right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/selection/selection-update-range-after-addrange.html:15:
assert_equals(selection.anchorNode, text);
Could you use assesrt_selection()? It is easier to verify where selection is
rather than having four assert_equals() calls.

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/selection/addRange-00-expected.txt
(right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/selection/addRange-00-expected.txt:1016:
FAIL Range 1 [paras[0].firstChild, 0, paras[0].firstChild, 1] followed by Range
1 [paras[0].firstChild, 0, paras[0].firstChild, 1]: second addRange() must
result in the selection's last range having the specified endpoints
assert_equals: startOffset of the Selection's last Range must match the added
Range expected 0 but got 6
Is this(PASS => FAIL) due by VP canonicalziation?

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:187: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
nit: Can we use early-return style?

if (didMoveDocument || checkForDifferentRootContainer(m_start, m_end)) {
  collapse(true);
  return;
}
updateSelectionIfAddedToSelection();

Should we detach Range from FrameSelection when Range move to another Document?

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:213: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
Similar to L187 in Range::setStart()

[yoichio, 2017-02-10 11:40:19]

On 2017/02/10 10:26:28, yosin_UTC9 wrote:
> Could you add difference of number of PASS and FAIL for explaining improvement
> from this patch?
> For example:
>   PASS +20
>   FAIL +3
I'm writing a patch that shows such result in -expected.txt:
https://codereview.chromium.org/2658793002/

[tkent, 2017-02-13 03:47:29]

On 2017/02/10 at 10:26:28, yosin wrote:
> Could you add difference of number of PASS and FAIL for explaining improvement
from this patch?
> For example:
>   PASS +20
>   FAIL +3

As we discussed offline, -expectedt.xt changes in WPT are ignorable even if this
increases FAILs.
Tests in wpt/selection are very stateful, and our Selection.addRange is
stateful.

On 2017/02/10 at 11:40:19, yoichio wrote:
> I'm writing a patch that shows such result in -expected.txt:
> https://codereview.chromium.org/2658793002/

Please land it :)

[tkent, 2017-02-13 04:04:13]

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:187: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
On 2017/02/10 at 10:26:27, yosin_UTC9 wrote:
> Should we detach Range from FrameSelection when Range move to another
Document?

Good point.  I'll investigate Firefox/Edge behavior.

[tkent, 2017-02-13 05:45:29]

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:187: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
On 2017/02/13 at 04:04:13, tkent wrote:
> On 2017/02/10 at 10:26:27, yosin_UTC9 wrote:
> > Should we detach Range from FrameSelection when Range move to another
Document?
> 
> Good point.  I'll investigate Firefox/Edge behavior.

Both of Firefox and Edge keep Selection.getRangeAt(0) even if the Range selects
detached Node or a Node owned by a different Document, and IDL attributes such
as Selection.anchorNode return values based on the Range.

Firefox clears visible text selection, and Edge keeps visible text selection
base on the previous value.  I think Firefox behavior is reasonable because
there are no ways to operate the visible text selection.
I'll update this CL.

[tkent, 2017-02-13 07:29:47]

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

[commit-bot: I haz the power, 2017-02-13 07:30:06]

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

[tkent, 2017-02-13 07:31:41]

Uploaded new patch set.

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/editing/selection/selection-update-range-after-addrange.html
(right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/selection/selection-update-range-after-addrange.html:15:
assert_equals(selection.anchorNode, text);
On 2017/02/10 at 10:26:27, yosin_UTC9 wrote:
> Could you use assesrt_selection()? It is easier to verify where selection is
rather than having four assert_equals() calls.

We can't test Range-Selection relationship behavior with assert_selection(),
which doesn't use Range at all.

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:187: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
On 2017/02/13 at 05:45:29, tkent wrote:
> On 2017/02/13 at 04:04:13, tkent wrote:
> > On 2017/02/10 at 10:26:27, yosin_UTC9 wrote:
> > > Should we detach Range from FrameSelection when Range move to another
Document?
> > 
> > Good point.  I'll investigate Firefox/Edge behavior.
> 
> Both of Firefox and Edge keep Selection.getRangeAt(0) even if the Range
selects detached Node or a Node owned by a different Document, and IDL
attributes such as Selection.anchorNode return values based on the Range.
> 
> Firefox clears visible text selection, and Edge keeps visible text selection
base on the previous value.  I think Firefox behavior is reasonable because
there are no ways to operate the visible text selection.
> I'll update this CL.

We discussed offline, and agreed that we should unregister the Range from the
Selection at this moment.

https://codereview.chromium.org/2687273002/diff/10001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Range.cpp:213: if (didMoveDocument ||
checkForDifferentRootContainer(m_start, m_end))
On 2017/02/10 at 10:26:27, yosin_UTC9 wrote:
> Similar to L187 in Range::setStart()

Done.

[yosin_UTC9, 2017-02-13 08:05:21]

lgtm

[tkent, 2017-02-13 08:27:36]

On 2017/02/10 at 10:26:28, yosin wrote:
> Could you add difference of number of PASS and FAIL for explaining improvement
from this patch?
> For example:
>   PASS +20
>   FAIL +3

New PASS: 182
New FAIL: 19

I'll add them to the description.

[tkent, 2017-02-13 08:29:57]

Description was changed from

==========
Selection API: Mutating a Range object after adding it to Selection should
update Selection attributes.

BUG=490206
==========

to

==========
Selection API: Mutating a Range object after adding it to Selection should
update Selection attributes.

This CL fixes 182 sub-tests in external/wpt/selection/addRange-NN.html, however
it breaks 19 sub-tests.  This is due to the unpredictability of the current
Selection.addRange() implementation.

BUG=490206
==========

[commit-bot: I haz the power, 2017-02-13 08:45:38]

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

[commit-bot: I haz the power, 2017-02-13 08:45:39]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-02-13 08:48:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2017-02-13 08:48:21]

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-13 08:52:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486975681253140,
"parent_rev": "a8e0f8228466461fb0dbbce6eb4373625a39b787", "commit_rev":
"0cc18c1b352bd7a0e8107652399c823f5bb81196"}

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

Description was changed from

==========
Selection API: Mutating a Range object after adding it to Selection should
update Selection attributes.

This CL fixes 182 sub-tests in external/wpt/selection/addRange-NN.html, however
it breaks 19 sub-tests.  This is due to the unpredictability of the current
Selection.addRange() implementation.

BUG=490206
==========

to

==========
Selection API: Mutating a Range object after adding it to Selection should
update Selection attributes.

This CL fixes 182 sub-tests in external/wpt/selection/addRange-NN.html, however
it breaks 19 sub-tests.  This is due to the unpredictability of the current
Selection.addRange() implementation.

BUG=490206

Review-Url: https://codereview.chromium.org/2687273002
Cr-Commit-Position: refs/heads/master@{#449920}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cc18c1b352bd7a0e8107652399c...
==========

[commit-bot: I haz the power, 2017-02-13 08:53:31]

Committed patchset #3 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0cc18c1b352bd7a0e8107652399c...