1723195 - firefox is not announcing grid column header while chrome does

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[maowenxin72]

Attachment: firefox not announcing column header.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.106 Safari/537.36

Steps to reproduce:

open NVDA speech viewer.
go to https://www.ag-grid.com/react-grid/getting-started/ and for 1st example click open in stackbliz.
move focus around cells. notice column header is not announced.
use chrome, notice column header is announced.

Actual results:

column header is not announced.

Expected results:

column header is announced.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Disability Access APIs' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[James Teh [:Jamie]]

Confirmed. Distilled test case:

data:text/html,
<div role="grid">
  <div role="rowgroup">
    <div role="row">
      <div role="columnheader">a</div>
    </div>
  </div>
  <div role="presentation" style="height: 1px; overflow: auto;">
    <div role="rowgroup">
      <div role="row">
        <div role="gridcell">b
      </div>
    </div>
  </div>
</div>

The bug is triggered by the div wrapping the second rowgroup. Although it has role="presentation", it is scrollable, which (in Firefox) causes it to be focusable and thus must be exposed to a11y APIs. Firefox should ignore this div when calculating table information.

We fixed problems like this in bug 1455066, but it seems this is a case we didn't cover. I'm not sure exactly why yet.

Comment 3

[James Teh [:Jamie]]

This is fixed by the new CachedTableAccessible implementation, which is currently behind a pref.

Comment 4

[James Teh [:Jamie]]

Actually, this is broken again. It was probably fixed by CTW but regressed in bug 1793748.

The reason this breaks is that we don't use ARIARowAccessible if there is an intervening Accessible between the table and the row group. However, if we do use ARIARowAccessible (as we did before the fix for bug 1793748), we get into nasty situations because the ARIA table classes can't handle an intervening Accessible. CachedTableAccessible doesn't care about that implementation, but we still have to live with that non-caching code for a little while.

Eventually, once we're certain CTW is good, we should just remove the ARIA table classes. We can then switch Accessible::IsTable* to use IsGenericType (which checks the ARIA role map as well) rather than explicitly checking mGenericTypes.

If we want to fix it before we start deleting non-caching code, we could change CachedTableAccessible to depend entirely on roles rather than checking the IsTable* methods. It would need to consider MathML table roles as well.

Comment 5

[James Teh [:Jamie]]

This should be fixed by bug 1832228. However, tests need to be added here.

As well as the test case in comment 2 (generics between table and rows), we should also have a test for generics between rows and cells.

Comment 6

[James Teh [:Jamie]]

Durp. Intervening generics between rows and cells aren't fixed by bug 1832228 because I... uh... forgot to include that code in the commit. Here's a patch:

diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp
index 01a0a6a2370d2..9f5d3de4db18c 100644
--- a/accessible/base/nsAccessibilityService.cpp
+++ b/accessible/base/nsAccessibilityService.cpp
@@ -115,11 +115,15 @@ static LocalAccessible* MaybeCreateSpecificARIAAccessible(
       return nullptr;
     }
     // A cell must be in a row.
-    if (aContext->Role() != roles::ROW) {
+    const Accessible* parent = aContext;
+    if (parent->IsGeneric()) {
+      parent = parent->GetNonGenericParent();
+    }
+    if (!parent || parent->Role() != roles::ROW) {
       return nullptr;
     }
     // That row must be in a table, though there may be an intervening rowgroup.
-    Accessible* parent = aContext->GetNonGenericParent();
+    parent = parent->GetNonGenericParent();
     if (!parent) {
       return nullptr;
     }

Comment 7

[Nathan LaPré]

Hah, I had just written the test and was debugging why this wasn't working when I got your email. This works, thanks! Patch incoming soon.

Comment 8

[Nathan LaPré]

Attachment: Bug 1723195: Allow intervening generic accessibles between table rows and cells, r?Jamie

This commit ensures that we get the proper non-generic parent when creating a
grid cell accessible. Before this patch, we wouldn't see this grid cell as a
grid cell accessible. This patch also adds a test for this case, along with a
test for a similar case that inserts a generic accessible between the table
itself and a rowgroup.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb6adbea9cdd
Allow intervening generic accessibles between table rows and cells, r=Jamie

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/cb6adbea9cdd