Skip to content

Commit

Permalink
Ensure not to maintain AccessibleTable2 when fetching child nodes whe…
Browse files Browse the repository at this point in the history 
…n the underlying table has neither rows nor columns (#12360)

When creating a virtual buffer and NVDA encounters a table, it wil render the children of that table. It keeps a reference to the IAccessibleTable2 interface of the table until it has found a cell in that table. However, when a table contains no cells (i.e. when it has neither rows nor columns) and it encounters an inner table in that table, that inner table is ignored and table navigation does not work.

Description of how this pull request fixes the issue:
This pr ensures that when row and column count are 0, the IAccessibleTable2 reference isn't used when rendering children. Therefore, for every child, the logic that checks for IAccessibleTable2 will apply and issue #12359 no longer occurs.
As a bonus, I removed support for IAccessibleTable2 support. This support was left in the VBuf code to support older versions of Seamonkey. I verified that Seamonkey now uses a version of Gecko that also uses IAccessibleTable2.

Co-authored-by: buddsean <sean@nvaccess.org>
  • Loading branch information
@LeonarddeR @seanbudd
LeonarddeR and seanbudd authored May 12, 2021
1 parent 8938ca4 commit 11b9950
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 87 deletions.
124 changes: 39 additions & 85 deletions nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,21 +105,6 @@ static IAccessible2* IAccessible2FromIdentifier(int docHandle, int ID) {
return pacc2;
}

template<typename TableType> inline void fillTableCounts(VBufStorage_controlFieldNode_t* node, IAccessible2* pacc, TableType* paccTable) {
wostringstream s;
long count = 0;
// Fetch row and column counts and add them as attributes on this vbuf node.
if (paccTable->get_nRows(&count) == S_OK) {
s << count;
node->addAttribute(L"table-rowcount", s.str());
s.str(L"");
}
if (paccTable->get_nColumns(&count) == S_OK) {
s << count;
node->addAttribute(L"table-columncount", s.str());
}
}

inline int getTableIDFromCell(IAccessibleTableCell* tableCell) {
IUnknown* unk = NULL;
if (tableCell->get_table(&unk) != S_OK || !unk)
Expand All @@ -136,31 +121,6 @@ inline int getTableIDFromCell(IAccessibleTableCell* tableCell) {
return id;
}

inline void fillTableCellInfo_IATable(VBufStorage_controlFieldNode_t* node, IAccessibleTable* paccTable, const wstring& cellIndexStr) {
wostringstream s;
long cellIndex = _wtoi(cellIndexStr.c_str());
long row, column, rowExtents, columnExtents;
boolean isSelected;
// Fetch row and column extents and add them as attributes on this node.
if (paccTable->get_rowColumnExtentsAtIndex(cellIndex, &row, &column, &rowExtents, &columnExtents, &isSelected) == S_OK) {
s << row + 1;
node->addAttribute(L"table-rownumber", s.str());
s.str(L"");
s << column + 1;
node->addAttribute(L"table-columnnumber", s.str());
if (columnExtents > 1) {
s.str(L"");
s << columnExtents;
node->addAttribute(L"table-columnsspanned", s.str());
}
if (rowExtents > 1) {
s.str(L"");
s << rowExtents;
node->addAttribute(L"table-rowsspanned", s.str());
}
}
}

typedef HRESULT(STDMETHODCALLTYPE IAccessibleTableCell::*IATableCellGetHeaderCellsFunc)(IUnknown***, long*);
inline void fillTableHeaders(VBufStorage_controlFieldNode_t* node, IAccessibleTableCell* paccTableCell, const IATableCellGetHeaderCellsFunc getHeaderCells, const wstring& attribName) {
wostringstream s;
Expand Down Expand Up @@ -379,7 +339,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
VBufStorage_buffer_t* buffer,
VBufStorage_controlFieldNode_t* parentNode,
VBufStorage_fieldNode_t* previousNode,
IAccessibleTable* paccTable,
IAccessibleTable2* paccTable2,
long tableID,
const wchar_t* parentPresentationalRowNumber,
Expand Down Expand Up @@ -505,7 +464,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
LOG_DEBUG(L"pacc->get_states failed");
IA2States=0;
}
// Remove state_editible from tables as Gecko exposes it for ARIA grids which is not in the ARIA spec.
// Remove state_editable from tables as Gecko exposes it for ARIA grids which is not in the ARIA spec.
if(IA2States&IA2_STATE_EDITABLE&&role==ROLE_SYSTEM_TABLE) {
IA2States-=IA2_STATE_EDITABLE;
}
Expand Down Expand Up @@ -732,7 +691,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
}

// Handle table cell information.
IAccessibleTableCell* paccTableCell = NULL;
IAccessibleTableCell* paccTableCell = nullptr;
if(pacc->QueryInterface(IID_IAccessibleTableCell, (void**)&paccTableCell)!=S_OK) {
paccTableCell=nullptr;
}
Expand Down Expand Up @@ -764,56 +723,58 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
}
}

// For IAccessibleTable, we must always be passed the table interface by the caller.
// For IAccessibleTable2, we can always obtain the cell interface,
// which allows us to handle updates to table cells.
if (
paccTableCell || // IAccessibleTable2
(paccTable && (IA2AttribsMapIt = IA2AttribsMap.find(L"table-cell-index")) != IA2AttribsMap.end()) // IAccessibleTable
) {
if (paccTableCell) {
// IAccessibleTable2
this->fillTableCellInfo_IATable2(parentNode, paccTableCell);
if (!paccTable2) {
// This is an update; we're not rendering the entire table.
tableID = getTableIDFromCell(paccTableCell);
}
paccTableCell->Release();
paccTableCell = NULL;
} else // IAccessibleTable
fillTableCellInfo_IATable(parentNode, paccTable, IA2AttribsMapIt->second);
if (paccTableCell) {
this->fillTableCellInfo_IATable2(parentNode, paccTableCell);
if (!paccTable2) {
// This is an update; we're not rendering the entire table.
tableID = getTableIDFromCell(paccTableCell);
}
paccTableCell->Release();
paccTableCell = nullptr;
// tableID is the IAccessible2::uniqueID for paccTable.
s << tableID;
parentNode->addAttribute(L"table-id", s.str());
s.str(L"");
// We're now within a cell, so descendant nodes shouldn't refer to this table anymore.
paccTable = NULL;
paccTable2 = NULL;
paccTable2 = nullptr;
tableID = 0;
}
// Handle table information.
// Don't release the table unless it was created in this call.
bool releaseTable = false;
// If paccTable is not NULL, we're within a table but not yet within a cell, so don't bother to query for table info.
if (!paccTable2 && !paccTable) {
IAccessibleTable2* curNodePaccTable2 = nullptr;
// If paccTable2 is not NULL, we're within a table but not yet within a cell, so don't bother to query for table info.
if (!paccTable2) {
// Try to get table information.
pacc->QueryInterface(IID_IAccessibleTable2,(void**)&paccTable2);
if(!paccTable2)
pacc->QueryInterface(IID_IAccessibleTable,(void**)&paccTable);
if (paccTable2||paccTable) {
// We did the QueryInterface for paccTable, so we must release it after all calls that use it are done.
releaseTable = true;
pacc->QueryInterface(IID_IAccessibleTable2, (void**)&curNodePaccTable2);
if (curNodePaccTable2) {
// This is a table, so add its information as attributes.
if((IA2AttribsMapIt = IA2AttribsMap.find(L"layout-guess")) != IA2AttribsMap.end())
if((IA2AttribsMapIt = IA2AttribsMap.find(L"layout-guess")) != IA2AttribsMap.end()) {
parentNode->addAttribute(L"table-layout",L"1");
}
tableID = ID;
s << ID;
parentNode->addAttribute(L"table-id", s.str());
s.str(L"");
if(paccTable2)
fillTableCounts<IAccessibleTable2>(parentNode, pacc, paccTable2);
else
fillTableCounts<IAccessibleTable>(parentNode, pacc, paccTable);
// Fetch row and column counts and add them as attributes on the vbuf node.
long rowCount = 0;
if (curNodePaccTable2->get_nRows(&rowCount) == S_OK) {
s << rowCount;
parentNode->addAttribute(L"table-rowcount", s.str());
s.str(L"");
}
long colCount = 0;
if (curNodePaccTable2->get_nColumns(&colCount) == S_OK) {
s << colCount;
parentNode->addAttribute(L"table-columncount", s.str());
s.str(L"");
}
if (rowCount > 0 || colCount > 0) {
// This table has rows and columns.
// Maintain curNodePaccTable2 for child rendering until any table cells are found.
paccTable2 = curNodePaccTable2;
}
// Add the table summary if one is present and the table is visible.
if (isVisible &&
(!description.empty() && (tempNode = buffer->addTextFieldNode(parentNode, previousNode, description))) ||
Expand Down Expand Up @@ -939,7 +900,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
buffer,
parentNode,
previousNode,
paccTable,
paccTable2,
tableID,
presentationalRowNumber,
Expand Down Expand Up @@ -981,7 +941,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
buffer,
parentNode,
previousNode,
paccTable,
paccTable2,
tableID,
presentationalRowNumber,
Expand All @@ -1005,7 +964,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
buffer,
parentNode,
previousNode,
paccTable,
paccTable2,
tableID,
presentationalRowNumber,
Expand All @@ -1032,7 +990,6 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
buffer,
parentNode,
previousNode,
paccTable,
paccTable2,
tableID,
presentationalRowNumber,
Expand Down Expand Up @@ -1133,13 +1090,10 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf(
SysFreeString(IA2Text);
if(paccText)
paccText->Release();
if (releaseTable) {
if(paccTable2)
paccTable2->Release();
else
paccTable->Release();
if (curNodePaccTable2) {
curNodePaccTable2->Release();
curNodePaccTable2 = nullptr;
}

return parentNode;
}

Expand Down
1 change: 0 additions & 1 deletion nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class GeckoVBufBackend_t: public VBufBackend_t {
VBufStorage_buffer_t* buffer,
VBufStorage_controlFieldNode_t* parentNode,
VBufStorage_fieldNode_t* previousNode,
IAccessibleTable* paccTable=NULL,
IAccessibleTable2* paccTable2=NULL,
long tableID=0, const wchar_t* parentPresentationalRowNumber=NULL,
bool ignoreInteractiveUnlabelledGraphics=false
Expand Down
42 changes: 41 additions & 1 deletion tests/system/robot/chromeTests.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2020 NV Access Limited
# Copyright (C) 2020-2021 NV Access Limited, Leonard de Ruijter
# This file may be used under the terms of the GNU General Public License, version 2 or later.
# For more details see: https://www.gnu.org/licenses/gpl-2.0.html

Expand Down Expand Up @@ -379,3 +379,43 @@ def test_i12147():
actualSpeech,
"target 0 heading level 4"
)


def test_tableInStyleDisplayTable():
"""
Chrome treats nodes with `style="display: table"` as tables.
When a HTML style table is positioned in such a node, NVDA was previously unable to announce
table row and column count as well as provide table navigation for the inner table.
"""
_chrome.prepareChrome(
"""
<p>Paragraph</p>
<div style="display:table">
<table>
<thead>
<tr>
<th>First heading</th>
<th>Second heading</th>
</tr>
</thead>
<tbody>
<tr>
<td>First content cell</td>
<td>Second content cell</td>
</tr>
</tbody>
</table>
</div>
"""
)
# Jump to the table
actualSpeech = _chrome.getSpeechAfterKey("t")
_asserts.strings_match(
actualSpeech,
"table with 2 rows and 2 columns row 1 First heading column 1 First heading"
)
nextActualSpeech = _chrome.getSpeechAfterKey("control+alt+downArrow")
_asserts.strings_match(
nextActualSpeech,
"row 2 First content cell"
)
3 changes: 3 additions & 0 deletions tests/system/robot/chromeTests.robot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,6 @@ ARIA checkbox
i12147
[Documentation] New focus target should be announced if the triggering element is removed when activated
test_i12147
Table in style display: table
[Documentation] Properly announce table row/column count and working table navigation for a HTML table in a div with style display: table
test_tableInStyleDisplayTable
1 change: 1 addition & 0 deletions user_docs/en/changes.t2t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ Plus many other important bug fixes and improvements.
- NVDA no longer crashes when using English US grade 2 and expand to computer Braille at the cursor is on, when displaying certain content such as a URL in Braille. (#11754)
- It is again possible to report formatting information for the focused Excel cell using NVDA+F. (#11914)
- QWERTY input on Papenmeier braille displays that support it again works and no longer causes NVDA to randomly freeze. (#11944)
- In Chromium based browsers, several cases were solved where table navigation didn't work and NVDA didn't report the number of rows/columns of the table. (#12359)


== Changes for Developers ==
Expand Down

0 comments on commit 11b9950

Please sign in to comment.