|
|
DescriptionCANCELLED 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
Patch Set 1 #
Total comments: 8
Dependent Patchsets: Messages
Total messages: 21 (12 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ==========
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL.
xiaochengh@chromium.org changed reviewers: + tkent@chromium.org, yoichio@chromium.org
+tkent, yoichio
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?
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)?
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.
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.
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?
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.
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 ==========
I can't come up with a clean and simple patch that fixes this issue. Hence closing it. |