Accessible setSelection function should use VisiblePositions

Accessible setSelection function should use VisiblePositions

The accessible setSelection function tries to select text in the DOM based
on nodes and text offsets within the accessibility tree. However, this
was implemented assuming that the text offsets were Positions, but they
should actually be VisiblePositions, since typically redundant whitespace
is not displayed visibly and similarly not included in the accessible name
of text nodes.

BUG=602455
Committed: https://crrev.com/9618f0ed66c582e8791fa6d37459056698ec1778
Cr-Commit-Position: refs/heads/master@{#393722}

[dmazzoni, 2016-05-02 21:57:08]

dmazzoni@chromium.org changed reviewers:
+ leviw@chromium.org, nektar@chromium.org

[nektarios, 2016-05-02 22:09:59]

Thanks for fixing this!
I noticed the offsets didn't match back in August but didn't know that there was
a way to retrieve the visual offsets.
One request: Could you have a look at AXLayoutObject::selection() and check that
visual positions are used there as well? I went through it just now and this
seems to be the case, but it wouldn't hurt to have a second opinion and make
sure that our code from the get selection side is also working properly.

[nektarios, 2016-05-02 22:10:21]

lgtm

[dmazzoni, 2016-05-02 22:21:50]

On 2016/05/02 22:09:59, nektarios wrote:
> One request: Could you have a look at AXLayoutObject::selection() and check
that
> visual positions are used there as well? I went through it just now and this
> seems to be the case, but it wouldn't hurt to have a second opinion and make
> sure that our code from the get selection side is also working properly.

Yes, it looks right to me. I didn't notice any bugs that direction, just
when trying to set the selection.

[dmazzoni, 2016-05-02 22:22:57]

By the way, there are some corner cases that might not work both when setting
and getting the selection - like for elements that have accessible text
different than the text nodes in the DOM. We should write some tests where the
selection includes things like images with alt text and try to get those to work
well.

[David Tseng, 2016-05-02 23:38:55]

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

[David Tseng, 2016-05-02 23:38:55]

Thanks for the fix!

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

https://codereview.chromium.org/1942953003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/accessibility/set-selection-whitespace.html:38:
axTextNode2.setSelectedTextRange(0, 6);
Can you also test setting selection on the container itself (axEditable2)?

[dglazkov, 2016-05-13 18:09:21]

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

[dglazkov, 2016-05-13 18:09:21]

lgtm

[dmazzoni, 2016-05-13 18:11:10]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-05-13 18:11:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942953003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942953003/1

[commit-bot: I haz the power, 2016-05-13 18:54:14]

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

[commit-bot: I haz the power, 2016-05-13 18:54:15]

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

[dmazzoni, 2016-05-13 21:42:55]

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

https://codereview.chromium.org/1942953003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/accessibility/set-selection-whitespace.html:38:
axTextNode2.setSelectedTextRange(0, 6);
On 2016/05/02 23:38:55, David Tseng wrote:
> Can you also test setting selection on the container itself (axEditable2)?

Done.

[dmazzoni, 2016-05-13 21:52:43]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-13 21:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/1942953003/#ps20001
(title: "Add test for selecting whole element")

[commit-bot: I haz the power, 2016-05-13 21:53:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1942953003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1942953003/20001

[commit-bot: I haz the power, 2016-05-14 01:20:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-14 01:21:28]

Description was changed from

==========
Accessible setSelection function should use VisiblePositions

The accessible setSelection function tries to select text in the DOM based
on nodes and text offsets within the accessibility tree. However, this
was implemented assuming that the text offsets were Positions, but they
should actually be VisiblePositions, since typically redundant whitespace
is not displayed visibly and similarly not included in the accessible name
of text nodes.

BUG=602455
==========

to

==========
Accessible setSelection function should use VisiblePositions

The accessible setSelection function tries to select text in the DOM based
on nodes and text offsets within the accessibility tree. However, this
was implemented assuming that the text offsets were Positions, but they
should actually be VisiblePositions, since typically redundant whitespace
is not displayed visibly and similarly not included in the accessible name
of text nodes.

BUG=602455

Committed: https://crrev.com/9618f0ed66c582e8791fa6d37459056698ec1778
Cr-Commit-Position: refs/heads/master@{#393722}
==========

[commit-bot: I haz the power, 2016-05-14 01:21:29]

Patchset 2 (id:??) landed as
https://crrev.com/9618f0ed66c582e8791fa6d37459056698ec1778
Cr-Commit-Position: refs/heads/master@{#393722}