Complete table support in ChromeVox Next.

Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e. beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role nodes in the automation tree
- table announcements (on the table node) take care of announcing the table row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers. The plan is to re-apply the leaving container rule very sleectively since it is over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/66e9e66d87b799f02667ceb55fd9feacbc980853
Cr-Commit-Position: refs/heads/master@{#406594}

[David Tseng, 2016-07-08 19:09:05]

Description was changed from

==========
Complete table support in ChromeVox Next.
==========

to

==========
Complete table support in ChromeVox Next.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-07-08 19:54:50]

Description was changed from

==========
Complete table support in ChromeVox Next.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers, jumping to beginning/end of row/column/entire table

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-07-08 19:54:50]

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

[David Tseng, 2016-07-08 19:55:04]

Ready for a look.

[David Tseng, 2016-07-08 20:08:29]

Patchset #2 (id:20001) has been deleted

[David Tseng, 2016-07-08 20:08:49]

Patchset #1 (id:1) has been deleted

[David Tseng, 2016-07-11 17:28:04]

Description was changed from

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers, jumping to beginning/end of row/column/entire table

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e.
beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-07-11 17:31:41]

Patchset #2 (id:60001) has been deleted

[David Tseng, 2016-07-11 17:32:29]

Patchset #1 (id:40001) has been deleted

[dmazzoni, 2016-07-13 23:38:28]

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json
(right):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json:588:
"ctrlKey": [true],
Does it work to have ChromeVox shortcuts that don't involve Search?

I thought that the way the keyboard listener worked, anything involving Search
got sent to ChromeVox first, but anything else did not, so potentially any keys
not involving Search could get intercepted by another app.

Even if we didn't implement that, my preference would be to try something with
Search, like Search+Control+Arrows.

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:337:
AutomationPredicate.rowCol = function(start, opts) {
I'd maybe call the predicate tableNavigation or moveWithinTable or something
like that

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:339:
throw new Error('You must set either row or col to true');
indent

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:395:
return function(node) {
Why is this a predicate that returns a function rather than a helper function
that just finds the right cell? If you have a 500 x 500 table and you want to
move to the next cell in the row, is it going to search through 250,000 cells to
find the one that matches this predicate?

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:809:
var tableOpts = {};
Any reason not to make this more concise? t doesn't look like you're using 'dir'
below.

How about:

var tableOpts = {row: true, dir: Dir.BACKWARD};

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:971:
TEST_F('BackgroundTest', 'BasicTableCommands', function() {
Could you add a test where a cell is missing? For example, say you want to move
to the last cell but the table doesn't have that cell - I don't think we're
handling that case now but I think it should return the last cell that's
actually in the table rather than only jumping to cell (M, N) if it exists and
returning an error otherwise.

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(left):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:415:
leave: '@exited_container($role)'
I think it's particularly useful to have exited_container for "dialog", what do
you think about moving it to the leave: rule for the dialog role for now?

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:145:
msgId: 'role_column'
Why is "cell" speaking the "column" message?

[David Tseng, 2016-07-14 20:24:41]

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json
(right):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json:588:
"ctrlKey": [true],
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> Does it work to have ChromeVox shortcuts that don't involve Search?
> 
> I thought that the way the keyboard listener worked, anything involving Search
> got sent to ChromeVox first, but anything else did not, so potentially any
keys
> not involving Search could get intercepted by another app.
> 
> Even if we didn't implement that, my preference would be to try something with
> Search, like Search+Control+Arrows.

ChromeVox gets everything. The special behavior with search is whether we
capture or not.

So potentially the ctrl alt arrows would cause other behaviors in addition to
trying to move in a table.

I'm fine with search ctrl alt. We would just come closer to a modal feel as
practically speaking, sticky mode would be needed for table nav at that point.

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:337:
AutomationPredicate.rowCol = function(start, opts) {
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> I'd maybe call the predicate tableNavigation or moveWithinTable or something
> like that

Probably part of the confusion...it makes a predicate, it isn't one itself.

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:339:
throw new Error('You must set either row or col to true');
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> indent

Done.

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:395:
return function(node) {
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> Why is this a predicate that returns a function rather than a helper function
> that just finds the right cell? If you have a 500 x 500 table and you want to
> move to the next cell in the row, is it going to search through 250,000 cells
to
> find the one that matches this predicate?

How did you conclude that? This method makes a predicate that gets used in the
same way as a jump command. I changed the method comment referring to vertical
cell btw.

The built predicate gets applied in depth first order. Cells appear from left to
right, top to bottom moving forward through the tree. So, in your example, we
would move through the rest of the current cell, unwind, move to the next cell
and return it after syncing down to a leaf. Certainly not 250000.

The only case where we would scan the entire table is when moving to the last or
first cell of the table. I've changed that to move directly to the directed end
and using a different starting node to search.

In the current code, if performance is really a concern, we can introduce a way
to exit early from a find next with yet another predicate. I want to avoid
writing one off tree walking that makes assumptions about the role ordering and
structure of the tree.

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:809:
var tableOpts = {};
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> Any reason not to make this more concise? t doesn't look like you're using
'dir'
> below.
> 
> How about:
> 
> var tableOpts = {row: true, dir: Dir.BACKWARD};

dir gets used in both places; the first is to calculate the new table row/col
index; the second, which direction to move in the tree walker. I don't have a
strong preference other than the current code being easier to edit. Changed.

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:971:
TEST_F('BackgroundTest', 'BasicTableCommands', function() {
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> Could you add a test where a cell is missing? For example, say you want to
move
> to the last cell but the table doesn't have that cell - I don't think we're
> handling that case now but I think it should return the last cell that's
> actually in the table rather than only jumping to cell (M, N) if it exists and
> returning an error otherwise.

Done.

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(left):

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:415:
leave: '@exited_container($role)'
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> I think it's particularly useful to have exited_container for "dialog", what
do
> you think about moving it to the leave: rule for the dialog role for now?
> 

Leaving for another cl where I can do an audit on what might make sense (e.g.
alert dialogs, windows, etc).

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

https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:145:
msgId: 'role_column'
On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> Why is "cell" speaking the "column" message?

This is unused by the cell speech rule...removed.

[dmazzoni, 2016-07-18 04:19:42]

On 2016/07/14 at 20:24:41, dtseng wrote:
> ChromeVox gets everything. The special behavior with search is whether we
capture or not.
> 
> So potentially the ctrl alt arrows would cause other behaviors in addition to
trying to move in a table.

Yes, that's what I was worried about.

> I'm fine with search ctrl alt. We would just come closer to a modal feel as
practically speaking, sticky mode would be needed for table nav at that point.

OK, let's do that for now.

>
https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
> File
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
(right):
> 
>
https://codereview.chromium.org/2132123002/diff/100001/chrome/browser/resourc...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js:337:
AutomationPredicate.rowCol = function(start, opts) {
> On 2016/07/13 23:38:28, dmazzoni_ooo_until_7_20 wrote:
> > I'd maybe call the predicate tableNavigation or moveWithinTable or something
> > like that
> 
> Probably part of the confusion...it makes a predicate, it isn't one itself.

Ah, I see now.

> > Why is this a predicate that returns a function rather than a helper
function
> > that just finds the right cell? If you have a 500 x 500 table and you want
to
> > move to the next cell in the row, is it going to search through 250,000
cells to
> > find the one that matches this predicate?
> 
> How did you conclude that? This method makes a predicate that gets used in the
same way as a jump command. I changed the method comment referring to vertical
cell btw.
> 
> The built predicate gets applied in depth first order. Cells appear from left
to right, top to bottom moving forward through the tree. So, in your example, we
would move through the rest of the current cell, unwind, move to the next cell
and return it after syncing down to a leaf. Certainly not 250000.
> 
> The only case where we would scan the entire table is when moving to the last
or first cell of the table. I've changed that to move directly to the directed
end and using a different starting node to search.

Cool, that's what I was thinking, when jumping to the beginning or end.

[dmazzoni, 2016-07-18 04:21:23]

lgtm

[David Tseng, 2016-07-19 22:36:53]

Description was changed from

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e.
beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e.
beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-07-19 22:37:05]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-07-19 22:37:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2132123002/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-07-19 22:37:54]

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-19 22:37:56]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

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

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

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

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

[David Tseng, 2016-07-20 16:37:13]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-07-20 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-20 16:37:47]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-20 17:16:09]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-07-20 17:16:29]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 17:18:11]

Description was changed from

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e.
beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Complete table support in ChromeVox Next.

What works:
- ctrl+alt+arrows navigates amongst table cells
- Ctrl+Alt+Shift+arrows navigates to the boundaries of the table (i.e.
beginning/end of row/column)
- Search+Ctrl+Alt+Shift+Left/Right navigates to the first/last cell of the table
- output is simple; read the row column index
- simplify table commands by using fully describe (now added as Search+k) to
announce table content
- row headers are picked up from the row node
- column headers don't appear to work because we're not getting the column role
nodes in the automation tree
- table announcements (on the table node) take care of announcing the table
row/column count, and table headers

Left for future/possibly unneeded:
- announcing spans
- keys to query for headers

Note that as part of this change, we're now not announcing leaving containers.
The plan is to re-apply the leaving container rule very sleectively since it is
over verbose even in verbose mode most of  the time.

BUG=513725
TEST=chromevox_tests --gtest_filter=BackgroundTest.BasicTableCommands
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/66e9e66d87b799f02667ceb55fd9feacbc980853
Cr-Commit-Position: refs/heads/master@{#406594}
==========

[commit-bot: I haz the power, 2016-07-20 17:18:13]

Patchset 7 (id:??) landed as
https://crrev.com/66e9e66d87b799f02667ceb55fd9feacbc980853
Cr-Commit-Position: refs/heads/master@{#406594}