Support ARIA table attributes, skip layout tables, and don't get stuck on hidden cells#7410
Merged
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.
jcsteh
requested changes
Jul 19, 2017
| } | ||
|
|
||
| // Save off the old parent for later propagation of some attributes | ||
| VBufStorage_fieldNode_t* origParentNode=parentNode; |
Contributor
There was a problem hiding this comment.
This doesn't seem to be used.
| if (paccTable->get_nRows(&count) == S_OK) { | ||
| s << count; | ||
| node->addAttribute(L"table-physicalrowcount", s.str()); | ||
| node->addAttribute(L"table-rowcount", s.str()); |
Contributor
There was a problem hiding this comment.
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 |
Contributor
There was a problem hiding this comment.
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 |
Contributor
There was a problem hiding this comment.
Don't need pass here, since the docstring counts as a statement.
| @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. |
Contributor
There was a problem hiding this comment.
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
Collaborator
|
Worth mentioning a long standing but here.
expected, either nothing, or movement. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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