CANCELLED Non-laid-out node should not be considered as user-select:contain

CANCELLED Non-laid-out node should not be considered as user-select:contain

This patch makes the current fake implementation of isUserSelectContain()
slightly saner by returning false for non-laid-out nodes. This is also
a preparation for removing layout update calls after applying editing
commands: https://codereview.chromium.org/2729313002

BUG=702756
Test=VisibleUnitsTest.canonicalizeSelectOutOfFlatTree

[Xiaocheng, 2017-03-17 20:14:22]

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

[commit-bot: I haz the power, 2017-03-17 20:14:52]

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-17 21:42:45]

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

[commit-bot: I haz the power, 2017-03-17 21:42:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2017-03-18 01:50:42]

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

[commit-bot: I haz the power, 2017-03-18 01:51:21]

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-18 04:12:46]

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

[commit-bot: I haz the power, 2017-03-18 04:12:47]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-20 22:16:32]

Description was changed from

==========
Non-laid-out node should not be considered as user-select:contain

BUG=702756
==========

to

==========
Non-laid-out node should not be considered as user-select:contain

This patch makes the current fake implementation of isUserSelectContain()
slightly saner by returning false for non-laid-out nodes. This is also
a preparation for removing layout update calls after applying editing
commands: https://codereview.chromium.org/2729313002

BUG=702756
Test=VisibleUnitsTest.canonicalizeSelectOutOfFlatTree
==========

[Xiaocheng, 2017-03-20 22:16:42]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-03-20 22:16:42]

PTAL.

[Xiaocheng, 2017-03-21 03:49:23]

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

[Xiaocheng, 2017-03-21 03:49:24]

+tkent, yoichio

[yoichio, 2017-03-21 06:48:44]

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return false;
We want |isUserSelectContain| independent from layout.
Could you check sanity in callers?

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
Getting an node which is after of the node that doesn't exist in  
flat tree doesn't make sense.
Could you assert it in POsitionInFlatTree constructor?
yosin@, do you have any thoughts?

[yosin_UTC9, 2017-03-21 08:48:51]

It seems we need to improve PositionInFlatTree ctor about checking validity of
anchor node.

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return false;
On 2017/03/21 at 06:48:43, yoichio wrote:
> We want |isUserSelectContain| independent from layout.
> Could you check sanity in callers?

Fumm... It seems we want to introduce isDisplayHiddenOf(const Node&)

bool isDisplayHidden(const Node& node) {
  DCHECK(node.isConnected()) << node;
  DCHECK_GE(node.document().lifecylce().state(), DocumentLifecycle::StyleClean);
  ComputedStyle* style = node->computedStyle();
  // |style| can be null if
  // - |node| isn't element, and
  // - TBD: Not sure when computed style of non-element |node| is null.
  return style && style->display() == EDisplay::KNone;
}

Just introduce limited version of isDisplayNonde(const Node&)? Like:

isDisplayNone(const Node& node) {
  DCHECK(node.isConnected());
  DCEHCK(style clean);
  return !node.layoutObject();
}


BTW, display and user-select are orthogonal, we should introduce another
function e.g. shouldSkipNode(const Node&), or another good name
to check isDisplayNonde() and isUserSelectContent()

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
On 2017/03/21 at 06:48:43, yoichio wrote:
> Getting an node which is after of the node that doesn't exist in  
> flat tree doesn't make sense.
> Could you assert it in POsitionInFlatTree constructor?
> yosin@, do you have any thoughts?

Agree, we should not accept position not in flat tree.

It seems we can do this check in |canBeAnchorNode<EditingInFlatTreeeStrateg>().

bool canBeAnchorNode<EditingInFlatTreeeStrateg>(const Node& node) {
  DCHECK(!node.needsDistributionRecalc());
  if (!canBeAnchorNode<EditingStrateg>(node))
    return false;
  if (!node.canParticipateInFlatTree())
   return false;
  if (!node.isConnected()) - disconnected node can't be in flat tree
    return false; // How about shadow host in document fragment?
  traverse flat tree parent until null or document. Return true if we reach
document otherwise false.
}

In this test case, SELECT is descendant of shadow host, but not in flat tree.

BTW, who constructor PositionInFlatTree::afterNode(SELECT)?

[Xiaocheng, 2017-03-21 20:27:48]

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return false;
On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> On 2017/03/21 at 06:48:43, yoichio wrote:
> > We want |isUserSelectContain| independent from layout.
> > Could you check sanity in callers?
> 
> Fumm... It seems we want to introduce isDisplayHiddenOf(const Node&)
> 
> bool isDisplayHidden(const Node& node) {
>   DCHECK(node.isConnected()) << node;
>   DCHECK_GE(node.document().lifecylce().state(),
DocumentLifecycle::StyleClean);
>   ComputedStyle* style = node->computedStyle();
>   // |style| can be null if
>   // - |node| isn't element, and
>   // - TBD: Not sure when computed style of non-element |node| is null.
>   return style && style->display() == EDisplay::KNone;
> }
> 
> Just introduce limited version of isDisplayNonde(const Node&)? Like:
> 
> isDisplayNone(const Node& node) {
>   DCHECK(node.isConnected());
>   DCEHCK(style clean);
>   return !node.layoutObject();
> }
> 
> 
> BTW, display and user-select are orthogonal, we should introduce another
function e.g. shouldSkipNode(const Node&), or another good name
> to check isDisplayNonde() and isUserSelectContent()

I'm confused.

What should be the ultimate fix to this bug?

Declare that a node without parent in flat tree cannot be the anchor node of a
Before/AfterAnchor PositionInFlatTree, and stop callers from generating such
positions?

Or add more checks in the callers (for this bug,
adjustPositionForBackwardIteration)?

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> On 2017/03/21 at 06:48:43, yoichio wrote:
> > Getting an node which is after of the node that doesn't exist in  
> > flat tree doesn't make sense.
> > Could you assert it in POsitionInFlatTree constructor?
> > yosin@, do you have any thoughts?
> 
> Agree, we should not accept position not in flat tree.
> 
> It seems we can do this check in
|canBeAnchorNode<EditingInFlatTreeeStrateg>().
> 
> bool canBeAnchorNode<EditingInFlatTreeeStrateg>(const Node& node) {
>   DCHECK(!node.needsDistributionRecalc());
>   if (!canBeAnchorNode<EditingStrateg>(node))
>     return false;
>   if (!node.canParticipateInFlatTree())
>    return false;
>   if (!node.isConnected()) - disconnected node can't be in flat tree
>     return false; // How about shadow host in document fragment?
>   traverse flat tree parent until null or document. Return true if we reach
document otherwise false.
> }
> 
> In this test case, SELECT is descendant of shadow host, but not in flat tree.
> 
> BTW, who constructor PositionInFlatTree::afterNode(SELECT)?

It's found when trying to remove layout update from Editor::*appliedEditing.
editing/undo/redo_correct_selection.html hits this DCHECK.

HTML: <div contenteditable>foo</div><option><select></select></option>

Editor::reappliedEditing has ending selection S=#text"foo"@0,
E=SELECT@AfterAnchor, which is a valid SelectionInDOMTree

Currently, Editor::reappliedEditing canonicalizes this selection into ^foo|
before passing it to FrameSelection::setSelection.

However, if we pass the uncanonicalized selection directly to
FrameSelection::setSelection, it crashes in the later canonicalization, where it
tries to canonicalize SELECT@AfterAnchor in the flat tree for computing a
VSInFlatTree.

[yosin_UTC9, 2017-03-22 03:19:28]

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return false;
On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > We want |isUserSelectContain| independent from layout.
> > > Could you check sanity in callers?
> > 
> > Fumm... It seems we want to introduce isDisplayHiddenOf(const Node&)
> > 
> > bool isDisplayHidden(const Node& node) {
> >   DCHECK(node.isConnected()) << node;
> >   DCHECK_GE(node.document().lifecylce().state(),
DocumentLifecycle::StyleClean);
> >   ComputedStyle* style = node->computedStyle();
> >   // |style| can be null if
> >   // - |node| isn't element, and
> >   // - TBD: Not sure when computed style of non-element |node| is null.
> >   return style && style->display() == EDisplay::KNone;
> > }
> > 
> > Just introduce limited version of isDisplayNonde(const Node&)? Like:
> > 
> > isDisplayNone(const Node& node) {
> >   DCHECK(node.isConnected());
> >   DCEHCK(style clean);
> >   return !node.layoutObject();
> > }
> > 
> > 
> > BTW, display and user-select are orthogonal, we should introduce another
function e.g. shouldSkipNode(const Node&), or another good name
> > to check isDisplayNonde() and isUserSelectContent()
> 
> I'm confused.
> 
> What should be the ultimate fix to this bug?
Change canonicalization not to return a position in shadow host:
  <div id="host"><a>foo</a><b>bar</b></div>
Invalid: P(host, N), {After,Before}Children(host), {After,Before}Anchor(A),
{After,Before}Anchor(B)
Valid: P("foo", N), {After,Before}Children(A), {After,Before}Children(B)

> 
> Declare that a node without parent in flat tree cannot be the anchor node of a
Before/AfterAnchor PositionInFlatTree, and stop callers from generating such
positions?
Yes.

> Or add more checks in the callers (for this bug,
adjustPositionForBackwardIteration)?
We should not pass a position in shadow host to mostBackwardCaretPosition().

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):

https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > Getting an node which is after of the node that doesn't exist in  
> > > flat tree doesn't make sense.
> > > Could you assert it in POsitionInFlatTree constructor?
> > > yosin@, do you have any thoughts?
> > 
> > Agree, we should not accept position not in flat tree.
> > 
> > It seems we can do this check in
|canBeAnchorNode<EditingInFlatTreeeStrateg>().
> > 
> > bool canBeAnchorNode<EditingInFlatTreeeStrateg>(const Node& node) {
> >   DCHECK(!node.needsDistributionRecalc());
> >   if (!canBeAnchorNode<EditingStrateg>(node))
> >     return false;
> >   if (!node.canParticipateInFlatTree())
> >    return false;
> >   if (!node.isConnected()) - disconnected node can't be in flat tree
> >     return false; // How about shadow host in document fragment?
> >   traverse flat tree parent until null or document. Return true if we reach
document otherwise false.
> > }
> > 
> > In this test case, SELECT is descendant of shadow host, but not in flat
tree.
> > 
> > BTW, who constructor PositionInFlatTree::afterNode(SELECT)?
> 
> It's found when trying to remove layout update from Editor::*appliedEditing.
editing/undo/redo_correct_selection.html hits this DCHECK.
> 
> HTML: <div contenteditable>foo</div><option><select></select></option>
> 
> Editor::reappliedEditing has ending selection S=#text"foo"@0,
E=SELECT@AfterAnchor, which is a valid SelectionInDOMTree
Yes, this is valid for SelectionInDOMTree.
But, not valid for VisibleSeleciton or VisiblePosition. It should Positon("foo",
3).
OPTION, which is shadow host, should be skipped by canonicalization.

> Currently, Editor::reappliedEditing canonicalizes this selection into ^foo|
before passing it to FrameSelection::setSelection.
> 
> However, if we pass the uncanonicalized selection directly to
FrameSelection::setSelection, it crashes in the later canonicalization, where it
tries to canonicalize SELECT@AfterAnchor in the flat tree for computing a
VSInFlatTree.

Position::afterNode(SELECT) can't be in Flat Tree, since it isn't distributed in
flat tree.

Anyway, it seems we should change canonicalization to avoid DOM positions in
shadow host.

[Xiaocheng, 2017-03-22 03:47:40]

On 2017/03/22 at 03:19:28, yosin wrote:
>
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):
> 
>
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return false;
> On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> > On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > > We want |isUserSelectContain| independent from layout.
> > > > Could you check sanity in callers?
> > > 
> > > Fumm... It seems we want to introduce isDisplayHiddenOf(const Node&)
> > > 
> > > bool isDisplayHidden(const Node& node) {
> > >   DCHECK(node.isConnected()) << node;
> > >   DCHECK_GE(node.document().lifecylce().state(),
DocumentLifecycle::StyleClean);
> > >   ComputedStyle* style = node->computedStyle();
> > >   // |style| can be null if
> > >   // - |node| isn't element, and
> > >   // - TBD: Not sure when computed style of non-element |node| is null.
> > >   return style && style->display() == EDisplay::KNone;
> > > }
> > > 
> > > Just introduce limited version of isDisplayNonde(const Node&)? Like:
> > > 
> > > isDisplayNone(const Node& node) {
> > >   DCHECK(node.isConnected());
> > >   DCEHCK(style clean);
> > >   return !node.layoutObject();
> > > }
> > > 
> > > 
> > > BTW, display and user-select are orthogonal, we should introduce another
function e.g. shouldSkipNode(const Node&), or another good name
> > > to check isDisplayNonde() and isUserSelectContent()
> > 
> > I'm confused.
> > 
> > What should be the ultimate fix to this bug?
> Change canonicalization not to return a position in shadow host:
>   <div id="host"><a>foo</a><b>bar</b></div>
> Invalid: P(host, N), {After,Before}Children(host), {After,Before}Anchor(A),
{After,Before}Anchor(B)
> Valid: P("foo", N), {After,Before}Children(A), {After,Before}Children(B)
> 
> > 
> > Declare that a node without parent in flat tree cannot be the anchor node of
a Before/AfterAnchor PositionInFlatTree, and stop callers from generating such
positions?
> Yes.
> 
> > Or add more checks in the callers (for this bug,
adjustPositionForBackwardIteration)?
> We should not pass a position in shadow host to mostBackwardCaretPosition().
> 
>
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):
> 
>
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
> On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> > On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > > Getting an node which is after of the node that doesn't exist in  
> > > > flat tree doesn't make sense.
> > > > Could you assert it in POsitionInFlatTree constructor?
> > > > yosin@, do you have any thoughts?
> > > 
> > > Agree, we should not accept position not in flat tree.
> > > 
> > > It seems we can do this check in
|canBeAnchorNode<EditingInFlatTreeeStrateg>().
> > > 
> > > bool canBeAnchorNode<EditingInFlatTreeeStrateg>(const Node& node) {
> > >   DCHECK(!node.needsDistributionRecalc());
> > >   if (!canBeAnchorNode<EditingStrateg>(node))
> > >     return false;
> > >   if (!node.canParticipateInFlatTree())
> > >    return false;
> > >   if (!node.isConnected()) - disconnected node can't be in flat tree
> > >     return false; // How about shadow host in document fragment?
> > >   traverse flat tree parent until null or document. Return true if we
reach document otherwise false.
> > > }
> > > 
> > > In this test case, SELECT is descendant of shadow host, but not in flat
tree.
> > > 
> > > BTW, who constructor PositionInFlatTree::afterNode(SELECT)?
> > 
> > It's found when trying to remove layout update from Editor::*appliedEditing.
editing/undo/redo_correct_selection.html hits this DCHECK.
> > 
> > HTML: <div contenteditable>foo</div><option><select></select></option>
> > 
> > Editor::reappliedEditing has ending selection S=#text"foo"@0,
E=SELECT@AfterAnchor, which is a valid SelectionInDOMTree
> Yes, this is valid for SelectionInDOMTree.
> But, not valid for VisibleSeleciton or VisiblePosition. It should
Positon("foo", 3).
> OPTION, which is shadow host, should be skipped by canonicalization.
> 
> > Currently, Editor::reappliedEditing canonicalizes this selection into ^foo|
before passing it to FrameSelection::setSelection.
> > 
> > However, if we pass the uncanonicalized selection directly to
FrameSelection::setSelection, it crashes in the later canonicalization, where it
tries to canonicalize SELECT@AfterAnchor in the flat tree for computing a
VSInFlatTree.
> 
> Position::afterNode(SELECT) can't be in Flat Tree, since it isn't distributed
in flat tree.
> 
> Anyway, it seems we should change canonicalization to avoid DOM positions in
shadow host.

How about modifying SelectionEditor::updateCachedVisibleSelection... to use the
canonicalized VS (in DOM) to construct the VSInFlatTree?

[yosin_UTC9, 2017-03-22 04:19:06]

On 2017/03/22 at 03:47:40, xiaochengh wrote:
> On 2017/03/22 at 03:19:28, yosin wrote:
> >
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right):
> > 
> >
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/EditingUtilities.cpp:302: return
false;
> > On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> > > On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > > > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > > > We want |isUserSelectContain| independent from layout.
> > > > > Could you check sanity in callers?
> > > > 
> > > > Fumm... It seems we want to introduce isDisplayHiddenOf(const Node&)
> > > > 
> > > > bool isDisplayHidden(const Node& node) {
> > > >   DCHECK(node.isConnected()) << node;
> > > >   DCHECK_GE(node.document().lifecylce().state(),
DocumentLifecycle::StyleClean);
> > > >   ComputedStyle* style = node->computedStyle();
> > > >   // |style| can be null if
> > > >   // - |node| isn't element, and
> > > >   // - TBD: Not sure when computed style of non-element |node| is null.
> > > >   return style && style->display() == EDisplay::KNone;
> > > > }
> > > > 
> > > > Just introduce limited version of isDisplayNonde(const Node&)? Like:
> > > > 
> > > > isDisplayNone(const Node& node) {
> > > >   DCHECK(node.isConnected());
> > > >   DCEHCK(style clean);
> > > >   return !node.layoutObject();
> > > > }
> > > > 
> > > > 
> > > > BTW, display and user-select are orthogonal, we should introduce another
function e.g. shouldSkipNode(const Node&), or another good name
> > > > to check isDisplayNonde() and isUserSelectContent()
> > > 
> > > I'm confused.
> > > 
> > > What should be the ultimate fix to this bug?
> > Change canonicalization not to return a position in shadow host:
> >   <div id="host"><a>foo</a><b>bar</b></div>
> > Invalid: P(host, N), {After,Before}Children(host), {After,Before}Anchor(A),
{After,Before}Anchor(B)
> > Valid: P("foo", N), {After,Before}Children(A), {After,Before}Children(B)
> > 
> > > 
> > > Declare that a node without parent in flat tree cannot be the anchor node
of a Before/AfterAnchor PositionInFlatTree, and stop callers from generating
such positions?
> > Yes.
> > 
> > > Or add more checks in the callers (for this bug,
adjustPositionForBackwardIteration)?
> > We should not pass a position in shadow host to mostBackwardCaretPosition().
> > 
> >
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp (right):
> > 
> >
https://codereview.chromium.org/2760533004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/editing/VisibleUnitsTest.cpp:2047:
canonicalPositionOf(PositionInFlatTree::afterNode(select)));
> > On 2017/03/21 at 20:27:48, Xiaocheng wrote:
> > > On 2017/03/21 at 08:48:51, yosin_UTC9 wrote:
> > > > On 2017/03/21 at 06:48:43, yoichio wrote:
> > > > > Getting an node which is after of the node that doesn't exist in  
> > > > > flat tree doesn't make sense.
> > > > > Could you assert it in POsitionInFlatTree constructor?
> > > > > yosin@, do you have any thoughts?
> > > > 
> > > > Agree, we should not accept position not in flat tree.
> > > > 
> > > > It seems we can do this check in
|canBeAnchorNode<EditingInFlatTreeeStrateg>().
> > > > 
> > > > bool canBeAnchorNode<EditingInFlatTreeeStrateg>(const Node& node) {
> > > >   DCHECK(!node.needsDistributionRecalc());
> > > >   if (!canBeAnchorNode<EditingStrateg>(node))
> > > >     return false;
> > > >   if (!node.canParticipateInFlatTree())
> > > >    return false;
> > > >   if (!node.isConnected()) - disconnected node can't be in flat tree
> > > >     return false; // How about shadow host in document fragment?
> > > >   traverse flat tree parent until null or document. Return true if we
reach document otherwise false.
> > > > }
> > > > 
> > > > In this test case, SELECT is descendant of shadow host, but not in flat
tree.
> > > > 
> > > > BTW, who constructor PositionInFlatTree::afterNode(SELECT)?
> > > 
> > > It's found when trying to remove layout update from
Editor::*appliedEditing. editing/undo/redo_correct_selection.html hits this
DCHECK.
> > > 
> > > HTML: <div contenteditable>foo</div><option><select></select></option>
> > > 
> > > Editor::reappliedEditing has ending selection S=#text"foo"@0,
E=SELECT@AfterAnchor, which is a valid SelectionInDOMTree
> > Yes, this is valid for SelectionInDOMTree.
> > But, not valid for VisibleSeleciton or VisiblePosition. It should
Positon("foo", 3).
> > OPTION, which is shadow host, should be skipped by canonicalization.
> > 
> > > Currently, Editor::reappliedEditing canonicalizes this selection into
^foo| before passing it to FrameSelection::setSelection.
> > > 
> > > However, if we pass the uncanonicalized selection directly to
FrameSelection::setSelection, it crashes in the later canonicalization, where it
tries to canonicalize SELECT@AfterAnchor in the flat tree for computing a
VSInFlatTree.
> > 
> > Position::afterNode(SELECT) can't be in Flat Tree, since it isn't
distributed in flat tree.
> > 
> > Anyway, it seems we should change canonicalization to avoid DOM positions in
shadow host.
> 
> How about modifying SelectionEditor::updateCachedVisibleSelection... to use
the canonicalized VS (in DOM) to construct the VSInFlatTree?

It doesn't work when selection crossing tree boundary.

[Xiaocheng, 2017-03-22 22:34:47]

Description was changed from

==========
Non-laid-out node should not be considered as user-select:contain

This patch makes the current fake implementation of isUserSelectContain()
slightly saner by returning false for non-laid-out nodes. This is also
a preparation for removing layout update calls after applying editing
commands: https://codereview.chromium.org/2729313002

BUG=702756
Test=VisibleUnitsTest.canonicalizeSelectOutOfFlatTree
==========

to

==========
CANCELLED Non-laid-out node should not be considered as user-select:contain

This patch makes the current fake implementation of isUserSelectContain()
slightly saner by returning false for non-laid-out nodes. This is also
a preparation for removing layout update calls after applying editing
commands: https://codereview.chromium.org/2729313002

BUG=702756
Test=VisibleUnitsTest.canonicalizeSelectOutOfFlatTree
==========

[Xiaocheng, 2017-03-22 22:35:14]

I can't come up with a clean and simple patch that fixes this issue.

Hence closing it.