Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ARIA table attributes, skip layout tables, and don't get stuck on hidden cells #7410

Merged
merged 4 commits into from Aug 2, 2017

Conversation

@michaelDCurran
Copy link
Contributor

@michaelDCurran michaelDCurran commented Jul 17, 2017

Link to issue number:

Fixes #6652
Fixes #5655
Fixes #7382

Summary of the issue:

Tables are becoming more complex on the web. to ensure that they remain accessible, ARIA allows for adding attributes to tables, rows and cells which expose more logical coordinates to the user, as the physical table's coordinates may be affected by hidden or not yet loaded data.
NVDA needs to report the ARIA coordinates and counts to the user, yet at the same time ensure it is still navigating the physical topology of the table. NVDA should also ensure that it does not stop on cells hidden with aria-hidden.
Further to this, layout tables should never be included in table navigation commands.

Description of how this pull request fixes the issue:

  • Gecko browseMode: skip over hidden table cells in table navigation commands. MSHTML already did this.
  • No longer include layout tables in table navigation commands
  • IAccessible NVDAObject properties: rowNumber, columnNumber, rowCount and columnCount now expose the ARIA values if available, otherwise falling back to the original physical table information.
  • Gecko controlField attributes: table-rownumber, table-columnnumber, table-rowcount and table-columncount now expose the ARIA values if available, otherwising falling back to the original physical table information
  • New Gecko controlField attributes: table-physicalrownumber, table-physicalcolumnnumber, table-physicalrowcount and table-physicalcolumncount values always expose the physical table information no matter what the presentational values may be.
  • Added new class variables to BrowseModeDocumentTreeInterceptor: navigationalTableRowNumberAttributeName and navigationalTableColumnNumberAttributeName which are used by BrowseModeDocumentTreeInterceptor._getTableCellCoords in place of the literal strings "table-rownumber" and "table-columnnumber" respectively, to allow fetching of the correct attributes containing physical table information.
  • Gecko_ia2 VirtualBuffer: override navigationalTableRowNumberAttributeName and navigationalTableColumnNumberAttributeName To provide the use of table-physicalrownumber and table-physicalcolumnnumber.

Testing performed:

Known issues with pull request:

  • ARIA table attributes not supported in Internet Explorer as it is ARIA 1.1. However, it would not be that hard to add them.
  • Chrome still has issues when navigating tables with hidden cells due to the coordinates that its accessibleTable interface accepts. It seems as though it is not returning a table cell when the cell is hidden with aria-hidden="true". Also some cells with display:None can confuse column and row counts.

Change log entry:

TBD

…ko / Chrome for both browse mode and focus mode.

Specifically:
* IAccessible NVDAObject properties: rowNumber, columnNumber, rowCount and columnCount now expose the ARIA values if available, otherwise falling back to the original physical table information.
* Gecko controlField attributes: table-rownumber, table-columnnumber, table-rowcount and table-columncount now expose the ARIA values if available, otherwising falling back to the original physical table information
*  New Gecko controlField attributes: table-physicalrownumber, table-physicalcolumnnumber, table-physicalrowcount and table-physicalcolumncount values always expose the physical table information no matter what the presentational values may be.
* Added new class variables to BrowseModeDocumentTreeInterceptor: navigationalTableRowNumberAttributeName and navigationalTableColumnNumberAttributeName which are used by BrowseModeDocumentTreeInterceptor._getTableCellCoords in place of  the literal strings "table-rownumber" and "table-columnnumber" respectively, to allow fetching of the correct attributes containing physical table information.
* Gecko_ia2 VirtualBuffer: override navigationalTableRowNumberAttributeName and navigationalTableColumnNumberAttributeName To provide the use of table-physicalrownumber and table-physicalcolumnnumber.
@@ -342,6 +348,9 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
return NULL;
}

// Save off the old parent for later propagation of some attributes
VBufStorage_fieldNode_t* origParentNode=parentNode;

This comment has been minimized.

@jcsteh

jcsteh Jul 19, 2017
Contributor

This doesn't seem to be used.

@@ -80,11 +80,13 @@ template<typename TableType> inline void fillTableCounts(VBufStorage_controlFiel
long count = 0;
if (paccTable->get_nRows(&count) == S_OK) {
s << count;
node->addAttribute(L"table-physicalrowcount", s.str());
node->addAttribute(L"table-rowcount", s.str());

This comment has been minimized.

@jcsteh

jcsteh Jul 19, 2017
Contributor

It'd be good to have a comment here explaining that although we default table-row/columncount to physical, this might be overridden by ARIA attributes later in fillVBuf.


return self._getTableCellAt(tableID,startPos,destRow,destCol)
# Try and fetch the cell at these coordinates, though if a hidden cell is hit, try up to 4 more times moving the coordinates on by one cell each time
limit=5

This comment has been minimized.

@jcsteh

jcsteh Jul 19, 2017
Contributor

Please make this a constant, as it seems reasonable that something might want to tweak this.


class HiddenCellFound(LookupError):
""" Raised when a table navigation method locates a cell but it is hidden, allowing the caller to possibly skip over it."""
pass

This comment has been minimized.

@jcsteh

jcsteh Jul 19, 2017
Contributor

Don't need pass here, since the docstring counts as a statement.

@@ -1576,6 +1592,7 @@ def _getTableCellAt(self,tableID,startPos,row,column):
@type column: int
@returns: the table cell's position in the document
@rtype: L{textInfos.TextInfo}
@raises: L{HiddenCellFound} if the cell it founds is hidden, allowing the caller to try a different cell.

This comment has been minimized.

@jcsteh

jcsteh Jul 19, 2017
Contributor

I'm concerned that we might not actually be able to tell the difference between a hidden cell and a cell which is out of bounds. Chrome doesn't render aria-hidden content in the tree at all, for example, and Gecko will probably do the same (eventually). Is there any reason we shouldn't just use LookupError here?

* In the gecko vbufBackend: comment why two sets of table attributes are being added.
* browseModeDocumentTreeInterceptor._getNearestTableCell: make the retry limit configurable via a _missingTableCellSearchLimit property.
* browseModeDocumentTreeInterceptor._getNearestTableCell: try more cells for any LookupError not just hiddenTableCellFound (browsers such as chrome do not expose hidden cells).
* remove browseMode.HiddenTableCellFound
* Gecko_ia2 virtualBuffer's _getTableCellAt: raise LookupError if a hidden cell is found rather than HiddenTableCellFound
@michaelDCurran michaelDCurran requested a review from jcsteh Jul 19, 2017
@jcsteh
jcsteh approved these changes Jul 19, 2017
Copy link
Contributor

@jcsteh jcsteh left a comment

Nice.

michaelDCurran added a commit that referenced this pull request Jul 19, 2017
@derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Aug 1, 2017

Worth mentioning a long standing but here.

  1. go to example 2 of https://www.w3.org/TR/wai-aria-practices/examples/grid/dataGrids.html
  2. switch to focus mode. after focusing 1,1
  3. use table nav commands.

expected, either nothing, or movement.
Actual: movement is allowed left and right, but up/down do nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment