New issue
Advanced search Search tips

Issue 803160 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug
STS
Team-Accessibility



Sign in to add a comment

[Select-to-Speak] STS needs to accept more varied types of selection

Project Member Reported by katie@chromium.org, Jan 17 2018

Issue description

Example page:
data:text/html,<br/><p>Selected text</p><br/>

Double-click on the text to highlight the region. The selection is:

anchorObject: staticText node of "Selected text"
anchorOffset: 0

focusObject: rootWebArea
focusOffset: 16

Select-to-speak gets the order of the selected nodes using AutomationUtil.getDirection, so it knows which comes first. But here the order is "backwards", which is incorrect since the rootWebArea selected region is *after* the staticText.

 

Comment 1 by katie@chromium.org, Jan 17 2018

Status: Started (was: Available)

Comment 2 by katie@chromium.org, Jan 17 2018

Summary: [Select-to-Speak] Direction checking can result in the wrong ordering in STS (was: [Select-to-Speak] AutomationUtil.getDirection is not sufficient when the focusObject is the root of the anchorObject)

Comment 3 by katie@chromium.org, Jan 17 2018

Owner: katie@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 20 2018

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

commit 5c62f84aed86066438f9c3e4e8497589e7b1a1e6
Author: Katie D <katie@chromium.org>
Date: Sat Jan 20 01:46:05 2018

Fix direction checking in Select-to-Speak.

If the anchor and focus object are the same node, or if one
is the root of the other, direction checking would be incorrect.
This makes sure that node ordering is correct.

Bug:  803160 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I0be73f4c307d47326bff557ada6918f0361038dc
Reviewed-on: https://chromium-review.googlesource.com/871844
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530727}
[modify] https://crrev.com/5c62f84aed86066438f9c3e4e8497589e7b1a1e6/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
[modify] https://crrev.com/5c62f84aed86066438f9c3e4e8497589e7b1a1e6/chrome/browser/resources/chromeos/select_to_speak/select_to_speak_keystroke_selection_test.extjs

Comment 5 by dtseng@chromium.org, Jan 20 2018

Meant to reply:
"order is "backwards", which is incorrect since the rootWebArea selected region is *after* the staticText."

What type of ordering did you expect here? The root web area is before the static text given a pre order traversal of the tree.

Anyhow, this is a bug in Blink selection. Focus nodes should not be before anchor nodes in document (i.e. pre order) traversal. The focus offset is also wrong. Focus offset of 16 here should be interpreted as a tree offset (i.e. 16th index into children of root web area), which doesn't exist.

These are both Blink bugs.
The first I think is fixed by
https://chromium-review.googlesource.com/c/chromium/src/+/562645
which hasn't been landed or updated.

Not sure about the second.

Comment 6 by katie@chromium.org, Feb 23 2018

Summary: [Select-to-Speak] STS needs to accept more varied types of selection (was: [Select-to-Speak] Direction checking can result in the wrong ordering in STS)
There's a lot more general selection issues with Select-to-Speak. STS needs to be more robust at getting the selected node deep equivalent position from the chrome.automation selection.

Right now it is possible to make a selection where the rootWebArea comes after the staticText, so I'm going to assume that until nektar@ says otherwise.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 26 2018

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

commit 941fec3de6c9e736ed2b342ffa5c7951774b9a83
Author: Katie D <katie@chromium.org>
Date: Mon Feb 26 19:52:13 2018

Refactors STS node processing code into node_utils.js.

select_to_speak.js was getting too messy so this moves
automation node related logic into a separate class.

Bug:  803160 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I6867864f5ccefc14f4a5b98a31af5220d16f8259
Reviewed-on: https://chromium-review.googlesource.com/937802
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539242}
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/BUILD.gn
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/compiled_resources2.gyp
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/manifest.json.jinja2
[add] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/node_utils.js
[add] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/node_utils_unittest.gtestjs
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/select_to_speak_keystroke_selection_test.extjs
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/browser/resources/chromeos/select_to_speak/select_to_speak_unittest.gtestjs
[modify] https://crrev.com/941fec3de6c9e736ed2b342ffa5c7951774b9a83/chrome/test/data/webui/BUILD.gn

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 1 2018

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

commit 407f08880d81826b09309bcc85c57e35a06b8306
Author: Katie D <katie@chromium.org>
Date: Thu Mar 01 02:07:48 2018

Working around selection quirks.

Selection can be messy if it goes across multiple types of containers.
This change is meant to make Select-to-Speak more robust to different
types of selections, and adds a lot of tests to help keep STS from
regressing in the future.

There are still several odd selection cases known that are not solved,
and any other strange selection tests would be appreciated.

There are also some small logic changes in select_to_speak function
to process selected nodes better.

Bug:  803160 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I015a58bae245bc5cab0f1f79b3c758d7456e1b37
Reviewed-on: https://chromium-review.googlesource.com/879091
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539985}
[modify] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/BUILD.gn
[modify] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/node_utils.js
[modify] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/node_utils_unittest.gtestjs
[add] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/pipe.jpg
[modify] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/select_to_speak.js
[modify] https://crrev.com/407f08880d81826b09309bcc85c57e35a06b8306/chrome/browser/resources/chromeos/select_to_speak/select_to_speak_keystroke_selection_test.extjs

Comment 9 by katie@chromium.org, Mar 5 2018

Status: Fixed (was: Started)
Marking this closed because it seems to have been completed for everything that's not an Automation bug that I can think of. For the automation selection bugs, see https://bugs.chromium.org/p/chromium/issues/list?can=2&q=reporter%3Ame+cc%3Anektar%40chromium.org+selection&colspec=ID+Pri+M+Stars+ReleaseBlock+Component+Status+Owner+Summary+OS+Modified&x=m&y=releaseblock&cells=ids.


Google Chrome 67.0.3369.0 (Official Build) canary (64-bit)
Firmware Version Google_Samus.6300.276.0
Flag enabled: #enable-experimental-accessibility-features

Example HTML above is read properly using both ways to invoke STS: highlight then search + s as well as search + drag highlight ring. 
Status: Verified (was: Fixed)
Components: UI>Accessibility>SelectToSpeak
Moving from just having STS label to also having the UI>Accessibility>SelectToSpeak component to make searching easier in the future. 

Sign in to add a comment