Use setSequentialFocusNavigationStartingPoint in ChromeVox

Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to the next link after that heading. Then navigate by line with Search+Up/Down and confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/fb6b8fd66c69ba0460fc38bbe3a256876665d7cf
Cr-Commit-Position: refs/heads/master@{#428313}

[dmazzoni, 2016-10-24 16:25:20]

Description was changed from

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

I can't think of a way to write an automated test for this since we
can't simulate a Tab key press from an extension test.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading.
==========

to

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

I can't think of a way to write an automated test for this since we
can't simulate a Tab key press from an extension test.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-10-24 16:25:30]

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

[dmazzoni, 2016-10-24 16:25:43]

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

[commit-bot: I haz the power, 2016-10-24 16:26:09]

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-10-24 18:34:22]

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

[commit-bot: I haz the power, 2016-10-24 18:34:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[David Tseng, 2016-10-24 21:51:32]

In this case, you could automate via a SpokenFeedbackTest.

https://codereview.chromium.org/2447773002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2447773002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:229:
setSequentialFocusNavigationStartingPoint,
This whole block is unused. Please add static methods below on AutomationNode.

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2172:
getNode()->document().clearFocusedElement();
Will this result in any ax events?

[dmazzoni, 2016-10-26 16:20:22]

Addressed feedback and added test.

Will refactor spoken feedback browser tests
as a follow-up.

https://codereview.chromium.org/2447773002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/automation.idl (right):

https://codereview.chromium.org/2447773002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/automation.idl:229:
setSequentialFocusNavigationStartingPoint,
On 2016/10/24 21:51:32, David Tseng wrote:
> This whole block is unused. Please add static methods below on AutomationNode.

Done.

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2172:
getNode()->document().clearFocusedElement();
On 2016/10/24 21:51:32, David Tseng wrote:
> Will this result in any ax events?

Yes, but the existing mechanism you added suppresses it from
speaking.

[dmazzoni, 2016-10-26 16:20:25]

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

[commit-bot: I haz the power, 2016-10-26 16:20:54]

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

[David Tseng, 2016-10-26 17:03:26]

Update TEST= with something like
manually navigated with ChromeVox by object, line. Verified tabbing moves from
that location rather than the previous focused item.

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2447773002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2172:
getNode()->document().clearFocusedElement();
On 2016/10/26 16:20:22, dmazzoni wrote:
> On 2016/10/24 21:51:32, David Tseng wrote:
> > Will this result in any ax events?
> 
> Yes, but the existing mechanism you added suppresses it from
> speaking.

Great, so you didn't get any repeated text/odd focus syncing while navigating
with ChromeVox.

https://codereview.chromium.org/2447773002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:776:
start.setSequentialFocusNavigationStartingPoint();
Does this work on in line text boxes?

[dmazzoni, 2016-10-26 17:30:32]

On 2016/10/26 17:03:26, David Tseng wrote:
> Update TEST= with something like
> manually navigated with ChromeVox by object, line. Verified tabbing moves from
> that location rather than the previous focused item.

I think it already says something like that? Is the example I gave bad?

https://codereview.chromium.org/2447773002/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:776:
> start.setSequentialFocusNavigationStartingPoint();
> Does this work on in line text boxes?

Yes, the implementation in Blink walks up to the parent and calls it on the
static text node that owns the inline text box.

[commit-bot: I haz the power, 2016-10-26 17:53:45]

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

[commit-bot: I haz the power, 2016-10-26 17:53:46]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-10-26 18:19:12]

On 2016/10/26 17:30:32, dmazzoni wrote:
> On 2016/10/26 17:03:26, David Tseng wrote:
> > Update TEST= with something like
> > manually navigated with ChromeVox by object, line. Verified tabbing moves
from
> > that location rather than the previous focused item.
> 
> I think it already says something like that? Is the example I gave bad?
> 

I was looking for line navigation (see comment below).
>
https://codereview.chromium.org/2447773002/diff/20001/chrome/browser/resource...
> >
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:776:
> > start.setSequentialFocusNavigationStartingPoint();
> > Does this work on in line text boxes?
> 
> Yes, the implementation in Blink walks up to the parent and calls it on the
> static text node that owns the inline text box.

Are you sure? focus() calls on inline textboxes still don't work (just verified
by
inlineTextBoxNode.focus();
chrome.automation.getFocus(function(focus) {window.focus = focus; });
focus remains where it was (somewhere totally at the beginning of the page).

In other words, you need to walk up here. Same thing happens for
setDocumentSelection.

[dmazzoni, 2016-10-26 21:24:25]

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

[commit-bot: I haz the power, 2016-10-26 21:25:02]

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

[dmazzoni, 2016-10-26 21:27:50]

Description was changed from

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

I can't think of a way to write an automated test for this since we
can't simulate a Tab key press from an extension test.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading. Then navigate by line with Search+Up/Down and
confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-26 22:49:21]

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

[commit-bot: I haz the power, 2016-10-26 22:49:22]

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-10-26 22:58:50]

Now handles focusing ancestors properly, and adds
another test.

[David Tseng, 2016-10-27 01:09:51]

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:786:
AutomationUtil.getDivergence(startAncestors, endAncestors));
As mentioned, AutomationUtil.getLeastCommonAncestor and then walk up.

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:788:
for (var i = commonAncestorCount - 1; i >= 0; --i) {
Thanks for doing this, but I don't really like the way this looks after
reconsidering.
- focusing root web areas blurs focus
- this messes up tab order. Example:
Suppose you have a toolbar (focusable) with 3 items. Two buttons and a text node
in that order. Object nav to the text. This would trigger a focus on the
toolbar. Tabbing at that point would move starting from the toolbar and onto the
first button. This goes for just about anything that's focusable with
descendants that are also focusable.

[dmazzoni, 2016-10-27 03:59:25]

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:788:
for (var i = commonAncestorCount - 1; i >= 0; --i) {
On 2016/10/27 01:09:50, David Tseng wrote:
> Thanks for doing this, but I don't really like the way this looks after
> reconsidering.
> - focusing root web areas blurs focus
> - this messes up tab order. Example:
> Suppose you have a toolbar (focusable) with 3 items. Two buttons and a text
node
> in that order. Object nav to the text. This would trigger a focus on the
> toolbar. Tabbing at that point would move starting from the toolbar and onto
the
> first button. This goes for just about anything that's focusable with
> descendants that are also focusable.

OK, what if we limit it to just links, and text fields?

I think if you navigate to content inside of a link or inside
of a contenteditable I'd expect it to get focus.

I understand your concern about containers like toolbars, so maybe we could
just special-case the roles that make sense.

[dmazzoni, 2016-10-27 06:27:09]

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

[dmazzoni, 2016-10-27 06:27:27]

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:786:
AutomationUtil.getDivergence(startAncestors, endAncestors));
On 2016/10/27 01:09:50, David Tseng wrote:
> As mentioned, AutomationUtil.getLeastCommonAncestor and then walk up.

Done.

https://codereview.chromium.org/2447773002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:788:
for (var i = commonAncestorCount - 1; i >= 0; --i) {
Switched to just links within the same frame, that seem reasonable?

[commit-bot: I haz the power, 2016-10-27 06:27:34]

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-10-27 08:02:37]

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

[commit-bot: I haz the power, 2016-10-27 08:02:38]

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

[dmazzoni, 2016-10-27 15:21:57]

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

[commit-bot: I haz the power, 2016-10-27 15:22:36]

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

[David Tseng, 2016-10-27 17:03:18]

https://codereview.chromium.org/2447773002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:792:
while (ancestor && ancestor.root == start.root) {
Counter example:

<a href="test">
  <button>ok</button>
  <p>with ChromeVox range here call focus on my parent</p>
<button>cancel</button>
  </a>

The correct way to focus here I think is to walk backwards in dfs order until
you find a focusable node. If that focusable node is an ancestor of the current
node/range, then focus it.

[commit-bot: I haz the power, 2016-10-27 17:13:38]

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

[commit-bot: I haz the power, 2016-10-27 17:13:39]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-10-27 19:42:50]

https://codereview.chromium.org/2447773002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/2447773002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:792:
while (ancestor && ancestor.root == start.root) {
On 2016/10/27 17:03:18, David Tseng wrote:
> Counter example:
> 
> <a href="test">
>   <button>ok</button>
>   <p>with ChromeVox range here call focus on my parent</p>
> <button>cancel</button>
>   </a>
> 
> The correct way to focus here I think is to walk backwards in dfs order until
> you find a focusable node. If that focusable node is an ancestor of the
current
> node/range, then focus it.

As discussed in person, NVDA behaves the same for this example
so I don't think it'd be wrong.

As discussed, I'm changing the code to only focus links and
controls. That avoids problems with cases like focusable
toolbars or focusable divs.

[dmazzoni, 2016-10-27 19:51:40]

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

[commit-bot: I haz the power, 2016-10-27 19:52:21]

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-10-27 21:26:48]

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

[commit-bot: I haz the power, 2016-10-27 21:26:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[David Tseng, 2016-10-27 22:42:38]

lgtm

https://codereview.chromium.org/2447773002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
(right):

https://codereview.chromium.org/2447773002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:103:
Role.colorWell,
Looks like this is just input type color. Needs an output rule (note fo me).

https://codereview.chromium.org/2447773002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:106:
Role.switch,
Didn't know about this role...
this one needs an output rule to mirror checkboxes (this is more of a note for
me).

[dmazzoni, 2016-10-27 22:56:02]

https://codereview.chromium.org/2447773002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
(right):

https://codereview.chromium.org/2447773002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:106:
Role.switch,
On 2016/10/27 22:42:38, David Tseng wrote:
> Didn't know about this role...
> this one needs an output rule to mirror checkboxes (this is more of a note for
> me).

What would you think if automation exposed a much smaller subset of roles,
plus a roleDescription?

We could map switch and checkbox to the same role, but with a different
role description. All the landmark roles would be the same, too - just one
role "landmark" plus a role description with the specific localized name.

[dmazzoni, 2016-10-27 22:56:10]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-10-27 22:57:10]

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-10-28 00:33:20]

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

[commit-bot: I haz the power, 2016-10-28 00:33:22]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dmazzoni, 2016-10-28 06:30:40]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-10-28 06:30:52]

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-10-28 07:54:07]

Description was changed from

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading. Then navigate by line with Search+Up/Down and
confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading. Then navigate by line with Search+Up/Down and
confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-28 07:54:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-28 07:58:29]

Description was changed from

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading. Then navigate by line with Search+Up/Down and
confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use setSequentialFocusNavigationStartingPoint in ChromeVox

Improves the interaction between ChromeVox navigation and input focus.
If you navigate to a focusable node, focuses it. If you navigate to a
descendant of a focusable node, focuses it too (checks the common
ancestors of the start and end of the selection). If not, sets the
sequential focus navigation start point to the start of the selection
so that subsequent focus using the Tab key is relative to that point.

Includes two new tests.

BUG=655272
TESTED=Visit Wikipedia, press Cvox+H to navigate to heading, then Tab to go to
the next link after that heading. Then navigate by line with Search+Up/Down and
confirm that it focuses links as you pass over them.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/fb6b8fd66c69ba0460fc38bbe3a256876665d7cf
Cr-Commit-Position: refs/heads/master@{#428313}
==========

[commit-bot: I haz the power, 2016-10-28 07:58:30]

Patchset 6 (id:??) landed as
https://crrev.com/fb6b8fd66c69ba0460fc38bbe3a256876665d7cf
Cr-Commit-Position: refs/heads/master@{#428313}

[David Tseng, 2016-11-04 15:38:18]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2479893002/ by dtseng@chromium.org.

The reason for reverting is: crbug.com/652140

This is impacting my ability to fix other bugs for m56.
.