Complete table support in ChromeVox Next.

chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js

Index: chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
diff --git a/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js b/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
index f054868b831967ccd7c8826b5d814b9260790741..065c60c36d6003338ab4c0f8b837fdce5d96e0f1 100644
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/automation_predicate.js
@@ -10,8 +10,11 @@ goog.provide('AutomationPredicate');
 goog.provide('AutomationPredicate.Binary');
 goog.provide('AutomationPredicate.Unary');
 
+goog.require('constants');
+
 goog.scope(function() {
 var AutomationNode = chrome.automation.AutomationNode;
+var Dir = constants.Dir;
 var RoleType = chrome.automation.RoleType;
 
 /**
@@ -283,10 +286,12 @@ AutomationPredicate.shouldIgnoreNode = function(node) {
   // Ignore some roles.
   return AutomationPredicate.leaf(node) &&
       (node.role == RoleType.client ||
+       node.role == RoleType.column ||
        node.role == RoleType.div ||
        node.role == RoleType.group ||
        node.role == RoleType.image ||
-       node.role == RoleType.staticText);
+       node.role == RoleType.staticText ||
+       node.role == RoleType.tableHeaderContainer);
 };
 
 
@@ -302,4 +307,96 @@ AutomationPredicate.checkable = function(node) {
       node.role == RoleType.menuItemRadio;
 };
 
+// Table related predicates.
+/**
+ * Returns if the node has a cell like role.
+ * @param {!AutomationNode} node
+ * @return {boolean}
+ */
+AutomationPredicate.cellLike = function(node) {
+  return node.role == RoleType.cell ||
+      node.role == RoleType.rowHeader ||
+      node.role == RoleType.columnHeader;
+};
+
+/**
+ * Returns a predicate that will match against the directed next vertical cell
+ * taking into account the current ancestor cell's position in the table.
+ * @param {AutomationNode} start
+ * @param {{dir: (Dir|undefined),
+ *          end: (boolean|undefined),
+*           row: (boolean|undefined),
+ *          col: (boolean|undefined)}} opts
+ * |dir|, specifies direction for |row or/and |col| movement by one cell.
+ *     |dir| defaults to forward.
+ *     |row| and |col| are both false by default.
+ *     |end| defaults to false. If set to true, the first row or co will be
+ *     returned.
+ * @return {?AutomationPredicate.Unary} Returns null if not in a table.
+ */
+AutomationPredicate.rowCol = function(start, opts) {

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

I'd maybe call the predicate tableNavigation or moveWithinTable or something
like that

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

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

+  if (!opts.row && !opts.col)
+  throw new Error('You must set either row or col to true');

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

indent

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

Done.

+
+  var dir = opts.dir || Dir.FORWARD;
+
+  // Compute the row/col index defaulting to 0.
+  var rowIndex = 0, colIndex = 0;
+  var tableNode = start;
+  while (tableNode) {
+    if (AutomationPredicate.table(tableNode))
+      break;
+
+    if (AutomationPredicate.cellLike(tableNode)) {
+      rowIndex = tableNode.tableCellRowIndex;
+      colIndex = tableNode.tableCellColumnIndex;
+    }
+
+    tableNode = tableNode.parent;
+  }
+  if (!tableNode)
+    return null;
+
+  if (!opts.end) {
+    // Adjust for the next/previous row/col.
+    if (opts.row)
+      rowIndex = dir == Dir.FORWARD ? rowIndex + 1 : rowIndex - 1;
+    if (opts.col)
+      colIndex = dir == Dir.FORWARD ? colIndex + 1 : colIndex - 1;
+  } else {
+    var rowEnd = dir == Dir.FORWARD ? (tableNode.tableRowCount - 1) : 0;
+    var colEnd = dir == Dir.FORWARD ? (tableNode.tableColumnCount - 1) : 0;
+
+    if (opts.row && opts.col) {
+      // Do nothing if we're already there.
+      if (rowEnd === rowIndex && colEnd === colIndex)
+        return null;
+
+      rowIndex = rowEnd;
+      colIndex = colEnd;
+    } else if (opts.row) {
+      // This looks counter-intuitive, but moving to the end of a row, means
+      // moving horizontally (by col).
+
+      // Do nothing if we're already there.
+      if (colEnd == colIndex)
+        return null;
+
+      colIndex = colEnd;
+    } else if (opts.col) {
+      // Do nothing if we're already there.
+      if (rowEnd == rowIndex)
+        return null;
+
+      rowIndex = rowEnd;
+    }
+  }
+
+  return function(node) {

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

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?

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

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.

+    return AutomationPredicate.cellLike(node) &&
+        node.tableCellColumnIndex == colIndex &&
+        node.tableCellRowIndex == rowIndex;
+  };
+};
+
 });  // goog.scope