New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 761173 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Oct 2017
Cc:
yosin@chromium.org
█ rlanday@chromium.org
editing-dev@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

PositionTemplate::IsConnected/IsOrphan don't work for flat tree

Project Member Reported by xiaoche...@chromium.org, Aug 31 2017 
The two functions are just wrappers of DOM-tree connectivity checks:

bool IsConnected() const { return anchor_node_ && anchor_node_->isConnected(); }

bool IsOrphan() const { return anchor_node_ && !anchor_node_->isConnected(); }

They work fine for Position, but not for PositionInFlatTree, because we may have an anchor node that is connected in DOM but disconnected in flat tree. For example, a child node of a shadow host that is not distributed into the shadow DOM.

There is a family of functions built on them, all of which are affected:

- PositionWithAffinityTemplate::IsOrphan/IsConnected
- VisiblePosition::IsOrphan
- SelectionTemplate::AssertValid/AssertValidFor(doc)
- VisibleSelectionTemplate::IsNonOrphanedCaretOrRange/IsValidFor(doc)

Comment 1 by yosin@chromium.org, Sep 1 2017

Components: -Blink>Editing Blink>Editing>Selection
PositionInFlatTree::IsConnected() requires ancestor traversal to document but
Position::IsConnected() uses cached value of Node::isConnect().

We may want to rename PositionTemplate::IsConnected() to ComputeIsConnected() or
another which denotes it isn't cheap since it requires O(depth).

I think it is hard to cache result of IsConnected() for flat tree.

IsConnected() isn't useful for editing because editing requires to have positions
in same document.

Thus, I propose IsOrpha/IsConnected to CheckValidFor(doc).
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 10 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c776b44dbdbc578fc5c05bdcd966251a50e9abe8

commit c776b44dbdbc578fc5c05bdcd966251a50e9abe8
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Oct 10 17:09:05 2017

Rewrite IsValidFor() for selection-like classes on top of PositionTemplate::IsValidFor()

This patch does the above mentioned change to reduce code duplication and fix
potential issues with IsOrphan() in flat tree.

Bug:  761173 
Change-Id: I673db3adb123a208d2d5c844c132ec4fe6bffd16
Reviewed-on: https://chromium-review.googlesource.com/707685
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507698}
[modify] https://crrev.com/c776b44dbdbc578fc5c05bdcd966251a50e9abe8/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp
[modify] https://crrev.com/c776b44dbdbc578fc5c05bdcd966251a50e9abe8/third_party/WebKit/Source/core/editing/VisibleSelection.cpp
[modify] https://crrev.com/c776b44dbdbc578fc5c05bdcd966251a50e9abe8/third_party/WebKit/Source/core/editing/commands/SelectionForUndoStep.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 11 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b45a13ace7952853b0302d454c337e0e3788f4f

commit 3b45a13ace7952853b0302d454c337e0e3788f4f
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Oct 11 05:48:14 2017

Fix PositionInFlatTree::IsConnected/IsOrphan

The above two functions checks only connectivity in DOM tree, which is
incorrect for flat tree positions. This patch corrects it.

Bug:  761173 
Change-Id: I4becd9a15cc6e0a794357040764847e2d8da418c
Reviewed-on: https://chromium-review.googlesource.com/707681
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507904}
[modify] https://crrev.com/3b45a13ace7952853b0302d454c337e0e3788f4f/third_party/WebKit/Source/core/editing/Position.cpp
[modify] https://crrev.com/3b45a13ace7952853b0302d454c337e0e3788f4f/third_party/WebKit/Source/core/editing/Position.h
[modify] https://crrev.com/3b45a13ace7952853b0302d454c337e0e3788f4f/third_party/WebKit/Source/core/editing/PositionTest.cpp

Comment 4 by xiaoche...@chromium.org, Oct 12 2017

Status: Fixed (was: Available)
►

Sign in to add a comment