Skip to content

Ensure not to maintain AccessibleTable2 when fetching child nodes when the underlying table has neither rows nor columns #12360

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

Merged
seanbudd merged 9 commits into from
May 12, 2021
Merged

Ensure not to maintain AccessibleTable2 when fetching child nodes when the underlying table has neither rows nor columns #12360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
88918a0
Remove support for IAccessibleTable
LeonarddeR May 3, 2021
a093157
Ensure not to maintain AccessibleTable2 when fetching child nodes whe…
LeonarddeR May 3, 2021
ad3f6f5
Try to provide a system test
LeonarddeR May 3, 2021
1212759
Make test work properly
LeonarddeR May 3, 2021
1ab1f03
Fix linting
LeonarddeR May 3, 2021
8992fc2
Update nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
LeonarddeR May 12, 2021
91b06e9
update changes
seanbudd May 12, 2021
bdec8f5
Merge branch 'master' into tableFix
seanbudd May 12, 2021
ceb489f
Merge branch 'tableFix' of github.com:leonardder/nvda into tableFix
seanbudd May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 39 additions & 85 deletions nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
View file
Open in desktop
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;
}
Comment on lines +760 to +777

@seanbudd seanbudd May 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this still be done in a helper function? It would be good to continue to split fillVBuf up.

@LeonarddeR LeonarddeR May 12, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be done in a helper function theoretically, but I'm afraid it will make the code less readable that way. Note that I removed this from a helper function because when it was a helper, it only fetched the row and column count to write it to the vbuf xml. Now the row and column count are used later on as well.

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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