Bug 270535 – AX: AXPropertyName::{ColumnIndex, RowIndex} are not updated on the isolated tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

RowIndex can become stale on the isolated table rows, causing issues with ATs. This is apparent when an author uses two THEADs, where we don't update the row index properly.

[Radar WebKit Bug Importer]

<rdar://problem/124092306>

[Joshua Hoffman]

rdar://118240861

[Joshua Hoffman]

Created attachment 470191 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=470191&action=review

> LayoutTests/accessibility/dynamic-table-row-column-indices.html:23
> +    var result = "";
> +    var index = 0;

var hoists the variable to the global scope, meaning it escapes the bounds of this function, which is usually not expected. Can we use let or const?

> LayoutTests/accessibility/dynamic-table-row-column-indices.html:26
> +        index++;

Is index ever used?

> LayoutTests/accessibility/dynamic-table-row-column-indices.html:48
> +    let newRow = document.createElement('tr');

Double quotes over single quotes here

> LayoutTests/accessibility/table-insert-second-thead.html:35
> +    let newTHEAD = document.createElement('thead');

Maybe "newTHead"? Also this line uses single quotes, we typically stick to double quotes in tests I think.

> LayoutTests/accessibility/table-insert-second-thead.html:46
> +        output += `\n${table.attributesOfRows()}`;
> +        output += `\n${table.attributesOfVisibleCells()}`;

attributesOfRows() and table.attributesOfVisibleCells() are convenient, but historically have resulted in hard to maintain and slow tests. See accessibility-isolated-tree/TestExpectations where we have skipped several tests because they unnecessarily log the frames and they are off by 1px. Are there more precise things we can verify about each row and cell, even if it makes this test a little long?

[Tyler Wilcock]

Comment on attachment 470191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=470191&action=review

> Source/WebCore/accessibility/AccessibilityTableColumn.cpp:84
> +    m_columnIndex = columnIndex;
> +
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    if (auto* cache = axObjectCache())
> +        cache->columnIndexChanged(*this);
> +#endif

We should return early if columnIndex == m_columnIndex so we don't unnecessarily post a notification. See AccessibilityTableCell::setRowIndex as an example.

> Source/WebCore/accessibility/AccessibilityTableRow.cpp:127
> +    m_rowIndex = rowIndex;
> +
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    if (auto* cache = axObjectCache())
> +        cache->rowIndexChanged(*this);
> +#endif

We should return early if m_rowIndex == m_rowIndex so we don't unnecessarily post a notification. See AccessibilityTableCell::setRowIndex as an example.

[Joshua Hoffman]

Created attachment 470193 [details]
Patch

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 470191 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=470191&action=review
> 
> > Source/WebCore/accessibility/AccessibilityTableColumn.cpp:84
> > +    m_columnIndex = columnIndex;
> > +
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> > +    if (auto* cache = axObjectCache())
> > +        cache->columnIndexChanged(*this);
> > +#endif
> 
> We should return early if columnIndex == m_columnIndex so we don't
> unnecessarily post a notification. See AccessibilityTableCell::setRowIndex
> as an example.
> 
> > Source/WebCore/accessibility/AccessibilityTableRow.cpp:127
> > +    m_rowIndex = rowIndex;
> > +
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> > +    if (auto* cache = axObjectCache())
> > +        cache->rowIndexChanged(*this);
> > +#endif
> 
> We should return early if m_rowIndex == m_rowIndex so we don't unnecessarily
> post a notification. See AccessibilityTableCell::setRowIndex as an example.

Thanks for all of your comments! Addressed them all in my latest patch. 

To your question about the `index` variable, that was leftover from a previous revision of that test, so I have removed all references to it.

[Joshua Hoffman]

Created attachment 470198 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=470198&action=review

> LayoutTests/accessibility/dynamic-table-row-column-indices.html:30
> +    for (let i = 0; i < table.rowCount; i++) {
> +        result += `Row Index [${i}]: ${table.rowAtIndex(i).indexInTable()}\n`;
> +    }   
> +
> +    const columns = table.columns();
> +    for (let i = 0; i < table.columnCount; i++) {
> +        result += `Column Index [${i}]: ${columns[i].indexInTable()}\n`;
> +    }   

The curly braces aren't necessary here, but not a huge deal.

[Joshua Hoffman]

Created attachment 470204 [details]
Patch

[EWS]

Committed 275762@main (1c3f222f4f96): <https://commits.webkit.org/275762@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 470204 [details].