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
Conversation
…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.
…mmands. MSHTML already did this.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
source/browseMode.py
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a constant, as it seems reasonable that something might want to tweak this.
source/browseMode.py
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need pass here, since the docstring counts as a statement.
source/browseMode.py
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Worth mentioning a long standing but here.
expected, either nothing, or movement. |
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:
Testing performed:
Known issues with pull request:
Change log entry:
TBD