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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cc163c
Add support for ARIA rowindex, colindex, rowcount and colcount in Gec…
michaelDCurran Jul 15, 2017
1d4519c
No longer include layout tables in table navigation commands
michaelDCurran Jul 16, 2017
e43a671
Gecko browseMode: skip over hidden table cells in table navigation co…
michaelDCurran Jul 17, 2017
a2b10a0
Address review actions:
michaelDCurran Jul 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 29 additions & 3 deletions nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
View file
Open in desktop
Expand Up @@ -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());
Copy link
Contributor

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.

s.str(L"");
}
if (paccTable->get_nColumns(&count) == S_OK) {
s << count;
node->addAttribute(L"table-physicalcolumncount", s.str());
node->addAttribute(L"table-columncount", s.str());
}
}
Expand Down Expand Up @@ -130,9 +132,11 @@ inline void fillTableCellInfo_IATable(VBufStorage_controlFieldNode_t* node, IAcc
boolean isSelected;
if (paccTable->get_rowColumnExtentsAtIndex(cellIndex, &row, &column, &rowExtents, &columnExtents, &isSelected) == S_OK) {
s << row + 1;
node->addAttribute(L"table-physicalrownumber", s.str());
node->addAttribute(L"table-rownumber", s.str());
s.str(L"");
s << column + 1;
node->addAttribute(L"table-physicalcolumnnumber", s.str());
node->addAttribute(L"table-columnnumber", s.str());
if (columnExtents > 1) {
s.str(L"");
Expand Down Expand Up @@ -188,9 +192,11 @@ inline void GeckoVBufBackend_t::fillTableCellInfo_IATable2(VBufStorage_controlFi
boolean isSelected;
if (paccTableCell->get_rowColumnExtents(&row, &column, &rowExtents, &columnExtents, &isSelected) == S_OK) {
s << row + 1;
node->addAttribute(L"table-physicalrownumber", s.str());
node->addAttribute(L"table-rownumber", s.str());
s.str(L"");
s << column + 1;
node->addAttribute(L"table-physicalcolumnnumber", s.str());
node->addAttribute(L"table-columnnumber", s.str());
if (columnExtents > 1) {
s.str(L"");
Expand Down Expand Up @@ -305,7 +311,7 @@ const wregex REGEX_PRESENTATION_ROLE(L"IAccessible2\\\\:\\\\:attribute_xml-roles

VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
VBufStorage_buffer_t* buffer, VBufStorage_controlFieldNode_t* parentNode, VBufStorage_fieldNode_t* previousNode,
IAccessibleTable* paccTable, IAccessibleTable2* paccTable2, long tableID,
IAccessibleTable* paccTable, IAccessibleTable2* paccTable2, long tableID, const wchar_t* parentPresentationalRowNumber,
bool ignoreInteractiveUnlabelledGraphics
) {
nhAssert(buffer); //buffer can't be NULL
Expand Down Expand Up @@ -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;
Copy link
Contributor

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.


//Add this node to the buffer
parentNode=buffer->addControlFieldNode(parentNode,previousNode,docHandle,ID,TRUE);
nhAssert(parentNode); //new node must have been created
Expand Down Expand Up @@ -655,6 +664,23 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
}
}

// Add some presentational table attributes
// Note these are only for reporting, the physical table attributes (table-physicalrownumber etc) for aiding in navigation etc are added later on.
// propagate table-rownumber down to the cell as Gecko only includes it on the row itself
if(parentPresentationalRowNumber)
parentNode->addAttribute(L"table-rownumber",parentPresentationalRowNumber);
const wchar_t* presentationalRowNumber=NULL;
if((IA2AttribsMapIt = IA2AttribsMap.find(L"rowindex")) != IA2AttribsMap.end()) {
parentNode->addAttribute(L"table-rownumber",IA2AttribsMapIt->second);
presentationalRowNumber=IA2AttribsMapIt->second.c_str();
}
if((IA2AttribsMapIt = IA2AttribsMap.find(L"colindex")) != IA2AttribsMap.end())
parentNode->addAttribute(L"table-columnnumber",IA2AttribsMapIt->second);
if((IA2AttribsMapIt = IA2AttribsMap.find(L"rowcount")) != IA2AttribsMap.end())
parentNode->addAttribute(L"table-rowcount",IA2AttribsMapIt->second);
if((IA2AttribsMapIt = IA2AttribsMap.find(L"colcount")) != IA2AttribsMap.end())
parentNode->addAttribute(L"table-columncount",IA2AttribsMapIt->second);

BSTR value=NULL;
if(pacc->get_accValue(varChild,&value)==S_OK) {
if(value&&SysStringLen(value)==0) {
Expand Down Expand Up @@ -736,7 +762,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
continue;
}
paccHyperlink->Release();
if (tempNode = this->fillVBuf(childPacc, buffer, parentNode, previousNode, paccTable, paccTable2, tableID, ignoreInteractiveUnlabelledGraphics)) {
if (tempNode = this->fillVBuf(childPacc, buffer, parentNode, previousNode, paccTable, paccTable2, tableID, presentationalRowNumber, ignoreInteractiveUnlabelledGraphics)) {
previousNode=tempNode;
} else {
LOG_DEBUG(L"Error in fillVBuf");
Expand Down Expand Up @@ -768,7 +794,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(IAccessible2* pacc,
VariantClear(&(varChildren[i]));
continue;
}
if (tempNode = this->fillVBuf(childPacc, buffer, parentNode, previousNode, paccTable, paccTable2, tableID, ignoreInteractiveUnlabelledGraphics))
if (tempNode = this->fillVBuf(childPacc, buffer, parentNode, previousNode, paccTable, paccTable2, tableID, presentationalRowNumber, ignoreInteractiveUnlabelledGraphics))
previousNode=tempNode;
else
LOG_DEBUG(L"Error in calling fillVBuf");
Expand Down
2 changes: 1 addition & 1 deletion nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.h
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ class GeckoVBufBackend_t: public VBufBackend_t {

VBufStorage_fieldNode_t* fillVBuf(IAccessible2* pacc,
VBufStorage_buffer_t* buffer, VBufStorage_controlFieldNode_t* parentNode, VBufStorage_fieldNode_t* previousNode,
IAccessibleTable* paccTable=NULL, IAccessibleTable2* paccTable2=NULL, long tableID=0,
IAccessibleTable* paccTable=NULL, IAccessibleTable2* paccTable2=NULL, long tableID=0, const wchar_t* parentPresentationalRowNumber=NULL,
bool ignoreInteractiveUnlabelledGraphics=false
);

Expand Down
36 changes: 32 additions & 4 deletions source/NVDAObjects/IAccessible/__init__.py
View file
Open in desktop
Expand Up @@ -1022,6 +1022,8 @@ def getChild(self, index):
return self.correctAPIForRelation(IAccessible(IAccessibleObject=child[0], IAccessibleChildID=child[1]))

def _get_IA2Attributes(self):
if not isinstance(self.IAccessibleObject,IAccessibleHandler.IAccessible2):
return {}
try:
attribs = self.IAccessibleObject.attributes
except COMError as e:
Expand All @@ -1035,7 +1037,7 @@ def event_IA2AttributeChange(self):
# We currently only care about changes to the accessible drag and drop attributes, which we map to states, so treat this as a stateChange.
self.event_stateChange()

def _get_rowNumber(self):
def _get_IA2PhysicalRowNumber(self):
table=self.table
if table:
if self.IAccessibleTableUsesTableCellIndexAttrib:
Expand All @@ -1052,7 +1054,15 @@ def _get_rowNumber(self):
log.debugWarning("IAccessibleTable::rowIndex failed", exc_info=True)
raise NotImplementedError

def _get_columnNumber(self):
def _get_rowNumber(self):
index=self.IA2Attributes.get('rowindex')
if index is None and isinstance(self.parent,IAccessible):
index=self.parent.IA2Attributes.get('rowindex')
if index is None:
index=self.IA2PhysicalRowNumber
return index

def _get_IA2PhysicalColumnNumber(self):
table=self.table
if table:
if self.IAccessibleTableUsesTableCellIndexAttrib:
Expand All @@ -1069,22 +1079,40 @@ def _get_columnNumber(self):
log.debugWarning("IAccessibleTable::columnIndex failed", exc_info=True)
raise NotImplementedError

def _get_rowCount(self):
def _get_columnNumber(self):
index=self.IA2Attributes.get('colindex')
if index is None:
index=self.IA2PhysicalColumnNumber
return index

def _get_IA2PhysicalRowCount(self):
if hasattr(self,'IAccessibleTableObject'):
try:
return self.IAccessibleTableObject.nRows
except COMError:
log.debugWarning("IAccessibleTable::nRows failed", exc_info=True)
raise NotImplementedError

def _get_columnCount(self):
def _get_rowCount(self):
count=self.IA2Attributes.get('rowcount')
if count is None:
count=self.IA2PhysicalRowCount
return count

def _get_IA2PhysicalColumnCount(self):
if hasattr(self,'IAccessibleTableObject'):
try:
return self.IAccessibleTableObject.nColumns
except COMError:
log.debugWarning("IAccessibleTable::nColumns failed", exc_info=True)
raise NotImplementedError

def _get_columnCount(self):
count=self.IA2Attributes.get('colcount')
if count is None:
count=self.IA2PhysicalColumnCount
return count

def _get__IATableCell(self):
# Permanently cache the result.
try:
Expand Down
50 changes: 40 additions & 10 deletions source/browseMode.py
View file
Open in desktop
Expand Up @@ -1539,6 +1539,11 @@ def script_movePastEndOfContainer(self,gesture):
# Translators: Description for the Move past end of container command in browse mode.
script_movePastEndOfContainer.__doc__=_("Moves past the end of the container element, such as a list or table")

#: The controlField attribute name that should be used as the row number when navigating in a table. By default this is the same as the presentational attribute name
navigationalTableRowNumberAttributeName="table-rownumber"
#: The controlField attribute name that should be used as the column number when navigating in a table. By default this is the same as the presentational attribute name
navigationalTableColumnNumberAttributeName="table-columnnumber"

def _getTableCellCoords(self, info):
"""
Fetches information about the deepest table cell at the given position.
Expand All @@ -1551,17 +1556,28 @@ def _getTableCellCoords(self, info):
if info.isCollapsed:
info = info.copy()
info.expand(textInfos.UNIT_CHARACTER)
for field in reversed(info.getTextWithFields()):
fields=list(info.getTextWithFields())
# First record the ID of all layout tables so that we can skip them when searching for the deepest table
layoutIDs=set()
for field in fields:
if isinstance(field, textInfos.FieldCommand) and field.command == "controlStart" and field.field.get('table-layout'):
tableID=field.field.get('table-id')
if tableID is not None:
layoutIDs.add(tableID)
for field in reversed(fields):
if not (isinstance(field, textInfos.FieldCommand) and field.command == "controlStart"):
# Not a control field.
continue
attrs = field.field
if "table-id" in attrs and "table-rownumber" in attrs:
tableID=attrs.get('table-id')
if tableID is None or tableID in layoutIDs:
continue
if self.navigationalTableColumnNumberAttributeName in attrs and not attrs.get('table-layout'):
break
else:
raise LookupError("Not in a table cell")
return (attrs["table-id"],
attrs["table-rownumber"], attrs["table-columnnumber"],
attrs[self.navigationalTableRowNumberAttributeName], attrs[self.navigationalTableColumnNumberAttributeName],
attrs.get("table-rowsspanned", 1), attrs.get("table-columnsspanned", 1))

def _getTableCellAt(self,tableID,startPos,row,column):
Expand All @@ -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.
Copy link
Contributor

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?

"""
raise NotImplementedError

Expand Down Expand Up @@ -1612,19 +1629,28 @@ def _getNearestTableCell(self, tableID, startPos, origRow, origCol, origRowSpan,
elif axis == "column":
destCol += origColSpan if movement == "next" else -1

if destCol < 1 or destRow<1:
# Optimisation: We're definitely at the edge of the column or row.
raise LookupError

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
Copy link
Contributor

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.

while limit>0:
limit-=1
if destCol < 1 or destRow<1:
# Optimisation: We're definitely at the edge of the column or row.
raise LookupError
try:
return self._getTableCellAt(tableID,startPos,destRow,destCol)
except HiddenCellFound:
pass
if axis=="row":
destRow+=1 if movement=="next" else -1
else:
destCol+=1 if movement=="next" else -1
raise LookupError

def _tableMovementScriptHelper(self, movement="next", axis=None):
if isScriptWaiting():
return
formatConfig=config.conf["documentFormatting"].copy()
formatConfig["reportTables"]=True
# For now, table movement includes layout tables even if reporting of layout tables is disabled.
formatConfig["includeLayoutTables"]=True
try:
tableID, origRow, origCol, origRowSpan, origColSpan = self._getTableCellCoords(self.selection)
except LookupError:
Expand Down Expand Up @@ -1702,3 +1728,7 @@ def _iterNotLinkBlock(self, direction="next", pos=None):
"kb:control+alt+rightArrow": "nextColumn",
"kb:control+alt+leftArrow": "previousColumn",
}

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
Copy link
Contributor

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.

14 changes: 14 additions & 0 deletions source/virtualBuffers/gecko_ia2.py
View file
Open in desktop
Expand Up @@ -19,10 +19,16 @@
import aria
import config
from NVDAObjects.IAccessible import normalizeIA2TextFormatField
import browseMode

class Gecko_ia2_TextInfo(VirtualBufferTextInfo):

def _normalizeControlField(self,attrs):
for attr in ("table-physicalrownumber","table-physicalcolumnnumber","table-physicalrowcount","table-physicalcolumncount"):
attrVal=attrs.get(attr)
if attrVal is not None:
attrs[attr]=int(attrVal)

current = attrs.get("IAccessible2::attribute_current")
if current is not None:
attrs['current']= current
Expand Down Expand Up @@ -260,12 +266,20 @@ def event_scrollingStart(self, obj, nextHandler):
return nextHandler()
event_scrollingStart.ignoreIsReady = True

# NVDA exposes IAccessible2 table interface row and column numbers as table-physicalrownumber and table-physicalcolumnnumber respectively.
# These should be used when navigating the physical table (I.e. these values should be provided to the table interfaces).
# The presentational table-columnnumber and table-rownumber attributes are normally duplicates of the physical ones, but are overridden by the values of aria-rowindex and aria-colindex if present.
navigationalTableRowNumberAttributeName="table-physicalrownumber"
navigationalTableColumnNumberAttributeName="table-physicalcolumnnumber"

def _getTableCellAt(self,tableID,startPos,destRow,destCol):
docHandle = self.rootDocHandle
table = self.getNVDAObjectFromIdentifier(docHandle, tableID)
try:
cell = table.IAccessibleTableObject.accessibleAt(destRow - 1, destCol - 1).QueryInterface(IAccessible2)
cell = NVDAObjects.IAccessible.IAccessible(IAccessibleObject=cell, IAccessibleChildID=0)
if cell.IA2Attributes.get('hidden'):
raise browseMode.HiddenCellFound
return self.makeTextInfo(cell)
except (COMError, RuntimeError):
raise LookupError
Expand Down