Fix visible position calculation in accessible setSelection

Fix visible position calculation in accessible setSelection

The accessible setSelection function is similar to the equivalent APIs to
create a range and set the selection in the DOM, except that offsets are
all visible positions, since the accessibility tree uses visible text
(with whitespace collapsed, for example).

The code that created a VisiblePosition from a text node and offset was
incorrect if the text node had a previous sibling. Fix it so that the
offset of the text node within its container is taken into account.

BUG=626385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/29605283e274ab1f520a07bce5507b691c117a55
Cr-Commit-Position: refs/heads/master@{#408520}

[dmazzoni, 2016-07-20 18:31:26]

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

[commit-bot: I haz the power, 2016-07-20 18:32:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2016-07-20 18:34:14]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, ojan@chromium.org

[dmazzoni, 2016-07-20 18:34:14]

ojan: I need your owners review for EditingUtilities.h

If you have time, there might be a better way for us to accomplish what we want.
This current fix seems a bit roundabout.

Is there a more straightforward way to create a VisiblePosition if I have
a text node and the index of a *visible position* in that node?

[nektarios, 2016-07-20 18:58:44]

We already have a function called
int AXLayoutObject::indexForVisiblePosition(const VisiblePosition& position)
const;
Now that blink::indexForVisiblePosition has been introduced, should we remove
it? Or, should we update that instead?

[nektarios, 2016-07-20 19:04:06]

Instead of returning bool from toVisiblePosition, could you return a null
visible position?

[commit-bot: I haz the power, 2016-07-20 19:26:37]

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

[commit-bot: I haz the power, 2016-07-20 19:26:38]

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

[dmazzoni, 2016-07-20 20:36:29]

On 2016/07/20 at 18:58:44, nektar wrote:
> We already have a function called
> int AXLayoutObject::indexForVisiblePosition(const VisiblePosition& position)
const;
> Now that blink::indexForVisiblePosition has been introduced, should we remove
it? Or, should we update that instead?

I didn't introduce it, blink::indexForVisiblePosition was already there and I
just exported it.

I'd like it if AXLayoutObject::indexForVisiblePosition could call
blink::indexForVisiblePosition, but blink::indexForVisiblePosition only takes a
ContainerNode, so you can't call it with a Text node directly. I'd like to
better understand why.

[David Tseng, 2016-07-20 20:39:15]

https://codereview.chromium.org/2164813003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/accessibility/set-selection-link.html
(right):

https://codereview.chromium.org/2164813003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/accessibility/set-selection-link.html:22: var
range = selection.getRangeAt(0);
Can we verify the selection data returned by ax (on the root ax node)?

[dmazzoni, 2016-07-20 20:40:25]

On 2016/07/20 at 19:04:06, nektar wrote:
> Instead of returning bool from toVisiblePosition, could you return a null
visible position?

Good idea, done.

[dmazzoni, 2016-07-20 20:42:23]

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

[commit-bot: I haz the power, 2016-07-20 20:43:08]

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, 2016-07-20 21:09:50]

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

[commit-bot: I haz the power, 2016-07-20 21:09:50]

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

[ojan, 2016-07-27 00:43:27]

ojan@chromium.org changed reviewers:
+ yoichio@chromium.org, yosin@chromium.org

[ojan, 2016-07-27 00:43:27]

yosin or yoichio, mind doing this review?

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

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/set-selection-link.html
(right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/set-selection-link.html:14: var
axPara = accessibilityController.accessibleElementById("para");
It is better to have

assert_not_equals(window.accessibilityController, undefined,
   'This test requires accessibilityController');

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/set-selection-link.html:28:
<script>
You don't need to have hide test sample. 
Or put |<div id="log"></div>|

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1994: 
Please add |DCHECK(node) << obj|.
For ease of debugging, it is better to introduce |AXObject* runner| to keep
input parameter |obj|.

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2007:
VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);
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]

Thanks for the review! I'm having trouble with the PlainTextRange suggestion,
below.

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/set-selection-link.html
(right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/set-selection-link.html:14: var
axPara = accessibilityController.accessibleElementById("para");
On 2016/07/27 at 01:31:14, Yosi_UTC9 wrote:
> It is better to have
> 
> assert_not_equals(window.accessibilityController, undefined,
>    'This test requires accessibilityController');

Done

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/set-selection-link.html:28:
<script>
On 2016/07/27 at 01:31:14, Yosi_UTC9 wrote:
> You don't need to have hide test sample. 
> Or put |<div id="log"></div>|

Removed.

Is there a way to have multiple ignored sections? This test
works fine, but I've had other tests where the HTML snippet
used in the test would mess up parsing the test results.

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:1994: 
On 2016/07/27 at 01:31:14, Yosi_UTC9 wrote:
> Please add |DCHECK(node) << obj|.
> For ease of debugging, it is better to introduce |AXObject* runner| to keep
input parameter |obj|.

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.

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2007:
VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);
On 2016/07/27 at 01:31:14, Yosi_UTC9 wrote:
> 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);

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]

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2007:
VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);
On 2016/07/27 at 06:45:17, dmazzoni wrote:
> On 2016/07/27 at 01:31:14, Yosi_UTC9 wrote:
> > 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);
> 
> 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.

I thinkg following code is identical to this sequence:

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

https://codereview.chromium.org/2164813003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2054:
frame->selection().setSelection(VisibleSelection(anchorVisiblePosition,
focusVisiblePosition));
Could you pass |Position| instead of |VisiblePositon|?
Since |VisiblePosition| constructor attempts to update layout, we would like to
avoid to reduce usage of VisaiblePosition.

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

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2007:
VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);
> I thinkg following code is identical to this sequence:
> 
> const PlaintTextRange& textRange(offset);
> const EphemeralRange& range = range.createRange(parent);
> return VisbilePosition(range.startPosition());

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]

lgtm

Sorry for confusion. 

It seems we should use indexForVisiblePosition() and visiblePositionForIndex(). 
Editing should provide better function than them...

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2164813003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2007:
VisiblePosition nodePosition = blink::visiblePositionBeforeNode(*node);
On 2016/07/27 at 07:49:22, dmazzoni wrote:
> > I thinkg following code is identical to this sequence:
> > 
> > const PlaintTextRange& textRange(offset);
> > const EphemeralRange& range = range.createRange(parent);
> > return VisbilePosition(range.startPosition());
> 
> 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.

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

[dmazzoni, 2016-07-27 16:37:15]

No problem, thanks.

If you end up adding new editing APIs in the future that might make
this easier, let me know.

[dmazzoni, 2016-07-27 16:37:25]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-07-27 16:37:45]

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, 2016-07-27 17:24:04]

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

[commit-bot: I haz the power, 2016-07-27 17:24:05]

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

[ojan, 2016-07-27 17:55:29]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-07-27 17:55:29]

lgtm

[commit-bot: I haz the power, 2016-07-27 17:55:51]

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, 2016-07-27 18:34:25]

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

[commit-bot: I haz the power, 2016-07-27 18:34:27]

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

[dmazzoni, 2016-07-28 21:21:44]

Description was changed from

==========
Fix visible position calculation in accessible setSelection

The accessible setSelection function is similar to the equivalent APIs to
create a range and set the selection in the DOM, except that offsets are
all visible positions, since the accessibility tree uses visible text
(with whitespace collapsed, for example).

The code that created a VisiblePosition from a text node and offset was
incorrect if the text node had a previous sibling. Fix it so that the
offset of the text node within its container is taken into account.

BUG=626385
==========

to

==========
Fix visible position calculation in accessible setSelection

The accessible setSelection function is similar to the equivalent APIs to
create a range and set the selection in the DOM, except that offsets are
all visible positions, since the accessibility tree uses visible text
(with whitespace collapsed, for example).

The code that created a VisiblePosition from a text node and offset was
incorrect if the text node had a previous sibling. Fix it so that the
offset of the text node within its container is taken into account.

BUG=626385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-07-28 21:28:49]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-07-28 21:28:50]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2164813003/#ps60001
(title: "Revert ChromeVox workaround and update test")

[commit-bot: I haz the power, 2016-07-28 21:29:13]

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, 2016-07-29 00:02:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 00:04:16]

Description was changed from

==========
Fix visible position calculation in accessible setSelection

The accessible setSelection function is similar to the equivalent APIs to
create a range and set the selection in the DOM, except that offsets are
all visible positions, since the accessibility tree uses visible text
(with whitespace collapsed, for example).

The code that created a VisiblePosition from a text node and offset was
incorrect if the text node had a previous sibling. Fix it so that the
offset of the text node within its container is taken into account.

BUG=626385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix visible position calculation in accessible setSelection

The accessible setSelection function is similar to the equivalent APIs to
create a range and set the selection in the DOM, except that offsets are
all visible positions, since the accessibility tree uses visible text
(with whitespace collapsed, for example).

The code that created a VisiblePosition from a text node and offset was
incorrect if the text node had a previous sibling. Fix it so that the
offset of the text node within its container is taken into account.

BUG=626385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/29605283e274ab1f520a07bce5507b691c117a55
Cr-Commit-Position: refs/heads/master@{#408520}
==========

[commit-bot: I haz the power, 2016-07-29 00:04:17]

Patchset 4 (id:??) landed as
https://crrev.com/29605283e274ab1f520a07bce5507b691c117a55
Cr-Commit-Position: refs/heads/master@{#408520}

[aboxhall, 2016-08-01 21:34:02]

aboxhall@chromium.org changed reviewers:
+ aboxhall@chromium.org

[aboxhall, 2016-08-01 21:34:03]

lgtm

[David Tseng, 2016-08-18 22:18:39]

This cl helps, but it still doesn't fix some of the cases that work in the DOM.
I've marked two places in the ChromeVox test. Also, indexing into children e.g.
selecting a non-leaf text node, doesn't always work.

Can we get a mini-spec going? I can write one up if needed, but it should pull
directly from the DOM's specification of how selection works.

https://codereview.chromium.org/2164813003/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs
(right):

https://codereview.chromium.org/2164813003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:400:
assertEquals(testNode, root.anchorObject);
FYI; this is still wrong.

https://codereview.chromium.org/2164813003/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/cursors_test.extjs:402:
assertEquals(4, root.anchorOffset);
As is this. The selection is on the ofSelection node.