Fix visible position calculation in accessible setSelection

third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp

Index: third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
diff --git a/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp b/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
index 3cf57b9846c1fbab4ead98a655ade10a90b46edd..82b9d2934d277128e9f3c42089dfe93ab2ed7de8 100644
--- a/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp
@@ -1982,6 +1982,33 @@ AXLayoutObject* AXLayoutObject::getUnignoredObjectFromNode(Node& node) const
 // Modify or take an action on an object.
 //
 
+// Convert from an accessible object and offset to a VisiblePosition.
+static VisiblePosition toVisiblePosition(AXObject* obj, int offset)
+{
+    // First walk up until we find an accessible object with an associated node.
+    Node* node = nullptr;
+    while (obj && !node) {
+        node = obj->getNode();
+        obj = obj->parentObject();
+    }
+

[yosin_UTC9, 2016/07/27 01:31:14]

Please add |DCHECK(node) << obj|.
For ease of debugging, it is better to introduce |AXObject* runner| to keep
input parameter |obj|.

[dmazzoni, 2016/07/27 06:45:17]

I changed to using a runner, good idea.

Rather than DCHECK, I'll just check for null |node| because I think there are
probably obscure cases where that could happen, like if this gets called during
teardown.

+    // If it's not a text node, no conversion is necessary, just create a VisiblePosition
+    // with this node and offset.
+    if (!node->isTextNode())
+        return createVisiblePosition(Position(node, offset));
+
+    // If it is a text node, we need to call some utility functions that use a TextIterator
+    // to walk the characters of the node and figure out the position corresponding to the
+    // visible character at position |offset|.
+    ContainerNode* parent = node->parentNode();
+    if (!parent)
+        return VisiblePosition();
+
+    VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);

[yosin_UTC9, 2016/07/27 01:31:14]

I recommend to use |PlainTextRange|, which is used by IME, to map text position
to DOM position.
If so, this function should return |Position| and rename to
|toPosition(AXObject* obj, int offset);

[dmazzoni, 2016/07/27 06:45:17]

I can't figure out how using PlainTextRange will make this easier.

Suppose that both the start and end of my selection are in different
Text nodes. In order to use PlainTextRange, I still need to pass it a
ContainerNode, which means I have to figure out the starting index of this node
within its container node.

Am I missing something?

I think the difference is that IME is always dealing with the plain text of an
editable text control. All of the selection offsets are relative to the root
editable element.

With accessibility we also have the case where the client wants to set a range
with an arbitrary start and end position, which can be in arbitrary text nodes
in the tree. I don't think PlainTextRange was meant for that.

I'm hoping there might be a better way than what I have now, though.

Another option would be to use a TextIterator, but that seems more low-level.

[yosin_UTC9, 2016/07/27 07:36:37]

I thinkg following code is identical to this sequence:

const PlaintTextRange& textRange(offset);
const EphemeralRange& range = range.createRange(parent);
return VisbilePosition(range.startPosition());

[dmazzoni, 2016/07/27 07:49:22]

I just tried it, it doesn't work because it assumes |node|
is the first child of |parent|. |offset| is an offset into
the text of |node|, but |parent| may have multiple children.

[yosin_UTC9, 2016/07/27 09:20:48]

I see, |offset| is visible offset in |node| instead of visible offset in
|parent|.

+    int nodeIndex = blink::indexForVisiblePosition(nodePosition, parent);
+    return blink::visiblePositionForIndex(nodeIndex + offset, parent);
+}
+
 void AXLayoutObject::setSelection(const AXRange& selection)
 {
     if (!getLayoutObject() || !selection.isValid())
@@ -2013,21 +2040,6 @@ void AXLayoutObject::setSelection(const AXRange& selection)
         return;
     }
 
-    Node* anchorNode = nullptr;
-    while (anchorObject && !anchorNode) {
-        anchorNode = anchorObject->getNode();
-        anchorObject = anchorObject->parentObject();
-    }
-
-    Node* focusNode = nullptr;
-    while (focusObject && !focusNode) {
-        focusNode = focusObject->getNode();
-        focusObject = focusObject->parentObject();
-    }
-
-    if (!anchorNode || !focusNode)
-        return;
-
     LocalFrame* frame = getLayoutObject()->frame();
     if (!frame)
         return;
@@ -2039,12 +2051,11 @@ void AXLayoutObject::setSelection(const AXRange& selection)
 
     // Set the selection based on visible positions, because the offsets in accessibility nodes
     // are based on visible indexes, which often skips redundant whitespace, for example.
-    VisiblePosition anchorVisiblePosition = anchorNode->isTextNode()
-        ? blink::visiblePositionForIndex(selection.anchorOffset, anchorNode->parentNode())
-        : createVisiblePosition(Position(anchorNode, selection.anchorOffset));
-    VisiblePosition focusVisiblePosition = focusNode->isTextNode()
-        ? blink::visiblePositionForIndex(selection.focusOffset, focusNode->parentNode())
-        : createVisiblePosition(Position(focusNode, selection.focusOffset));
+    VisiblePosition anchorVisiblePosition = toVisiblePosition(anchorObject, selection.anchorOffset);
+    VisiblePosition focusVisiblePosition = toVisiblePosition(focusObject, selection.focusOffset);
+    if (anchorVisiblePosition.isNull() || focusVisiblePosition.isNull())
+        return;
+
     frame->selection().setSelection(VisibleSelection(anchorVisiblePosition, focusVisiblePosition));
 }