Fix visible position calculation in accessible setSelection

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

 /*
 * Copyright (C) 2008 Apple Inc. All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 *
 * 1.  Redistributions of source code must retain the above copyright
 *     notice, this list of conditions and the following disclaimer.
 * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 1964 matching lines @@
         return toAXLayoutObject(axObject);
 
     return nullptr;
 }
 
 
 //
 // 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())
         return;
 
     AXObject* anchorObject = selection.anchorObject ?
         selection.anchorObject.get() : this;
     AXObject* focusObject = selection.focusObject ?
         selection.focusObject.get() : this;
 
@@ skipping 11 matching lines @@
                 selection.anchorOffset, selection.focusOffset,
                 SelectionHasForwardDirection, NotDispatchSelectEvent);
         } else {
             textControl->setSelectionRange(
                 selection.focusOffset, selection.anchorOffset,
                 SelectionHasBackwardDirection, NotDispatchSelectEvent);
         }
         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;
 
     // TODO(dglazkov): The use of updateStyleAndLayoutIgnorePendingStylesheets needs to be audited.
     // see http://crbug.com/590369 for more details.
     // This callsite should probably move up the stack.
     frame->document()->updateStyleAndLayoutIgnorePendingStylesheets();
 
     // 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));
 }
 
 bool AXLayoutObject::isValidSelectionBound(const AXObject* boundObject) const
 {
     return getLayoutObject() && boundObject && !boundObject->isDetached()
         && boundObject->isAXLayoutObject() && boundObject->getLayoutObject()
         && boundObject->getLayoutObject()->frame() == getLayoutObject()->frame()
         && &boundObject->axObjectCache() == &axObjectCache();
 }
@@ skipping 521 matching lines @@
                     result.unite(labelRect);
                 }
             }
         }
     }
 
     return result;
 }
 
 } // namespace blink