1730474 - All cells in a table are exposed as selectable, even without aria-selectable attribute

Status: ASSIGNED

(Firefox :: Disability Access, defect, P3)

Description

[Andy]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:92.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

Inspect accessibility properties of a generic HTML table

Actual results:

All cell and columnheader cells exposed state "selectable" in the accessibility tree.

Expected results:

Only widgets should expose "selectable" that are explicitly selectable by either
a) presence of the aria-selected attribute
b) implicit selection is possible due to focusable cells

In tables, rows maybe selectable instead of cells.

Comment 1

[Eitan Isaacson [:eeejay]]

Changing severity to S3 because this is not a regression but a defect that existed for a very long time. With the absence of a real-world example of an impact on users it isn't a high priority.

With that said, this is a real bug that should be fixed.

Comment 2

[gloriaeskor]

Hi, I am an outreachy applicant. Please can I work on this?

Comment 3

[Eitan Isaacson [:eeejay]]

gloriaeskor, for sure. Do you understand the work that needs to happen here, how to reproduce the bug, and how to verify it is fixed?

Once you have a general idea about how to go about fixing this, I can assign it to you.

Comment 4

[sirri]

:eeejay I will like to work on this issue. I think I understand the work that needs to happen. How do I reproduce the bug?

Comment 5

[gloriaeskor]

Can you please throw more light on what needs to be done?

Comment 6

[gloriaeskor]

I have read the description and reproduced the bug. Can you please throw more light on what needs to be done?

Comment 7

[Eitan Isaacson [:eeejay]]

This is where the SELECTABLE state is hard-coded:
https://searchfox.org/mozilla-central/source/accessible/html/HTMLTableAccessible.cpp#71

I think it should be removed, along with the NativeInteractiveState overriding method.

I looked at other browsers, and I don't believe we should ever have a SELECTABLE state on a non-aria-role table cell. So removing that method, and adding a test for it should be good enough!

Comment 8

[Eitan Isaacson [:eeejay]]

As for testing, I think that after that change we will have some test failures, we need to look at those failures and determine that they are false positives and indeed we are doing the right thing.

Comment 9

[Andy]

Thanks everybody for looking into this, as I see it this might finally make accessible data grids possible, allowing the concerned users to use complex apps. Currently, browser support in general of the aria-selected attribute in grids seems very low, Firefox could be the first (:

https://a11ysupport.io/tech/aria/aria-selected_attribute

For screen readers, the import things to know are

• whether an element under focus CAN be selected
• whether that element IS selected

The ARIA standard limits use of the aria-selected attribute to certain roles, but I'm not familiar enough with the accessibility tree or the inner workings of screen readers to know who's responsibility it is to make sure aria-selected is not used on other roles (author's or user agent's/browser's).

In my opinion there is not much harm done in exposing aria-selected info of <td> as soon as aria-selected is present, no matter the role. There is more value in finally exposing that state, than in limiting it's use to certain roles.

https://www.w3.org/TR/wai-aria-1.1/#aria-selected
https://www.w3.org/TR/wai-aria-1.1/#gridcell

Comment 10

[Andy]

Thanks everybody for looking into this, as I see it this might finally make accessible data grids possible, allowing the concerned users to use complex apps. Currently, browser support in general of the aria-selected attribute in grids seems very low, Firefox could be the first (:

https://a11ysupport.io/tech/aria/aria-selected_attribute

For screen readers, the import things to know are

• whether an element under focus CAN be selected
• whether that element IS selected

The ARIA standard limits use of the aria-selected attribute to certain roles, but I'm not familiar enough with the accessibility tree or the inner workings of screen readers to know who's responsibility it is to make sure aria-selected is not used on other roles (author's or user agent's/browser's).

In my opinion there is not much harm done in exposing aria-selected info of <td> as soon as aria-selected is present, no matter the role. There is more value in finally exposing that state, than in limiting it's use to certain roles.

https://www.w3.org/TR/wai-aria-1.1/#aria-selected
https://www.w3.org/TR/wai-aria-1.1/#gridcell

Comment 11

[Rares Doghi, Desktop QA]

I was able to reproduce this issue using a Wiki table from this page: https://en.wikipedia.org/wiki/List_of_Nvidia_graphics_processing_units in all versions of Firefox, In the Accessibility tree all cells have the "Selected" attribute.

Im not too familiar with the Accessibility Tree inspection so please correct me if this is not the actual issue stated here.

Comment 12

[Francis]

Hi, I'd like to work on this bug for my first open source contribution.

Comment 13

[Francis]

I've been working on the bug for a few days now, and I have been stuck on what necessary changes need to be made to make tests pass. I removed NativeInteractiveState method and ran the mochitest-a11y test suite to investigate failures - so far only test_sels_table.html was failing.

I have attempted to make it pass by removing STATE_SELECTABLE from the selected states test in table.js (https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/table.js#741). However, after this change, test_sels_table.html now failed since testStates() itself was checking whether the table cell element is selected (https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/states.js#245).

So: as we require in this bug that non-aria-role table cells should not be SELECTABLE, should we also be suppressing the SELECTED attribute for non-aria-role table cells?

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to Francis from comment #13)

So: as we require in this bug that non-aria-role table cells should not be SELECTABLE, should we also be suppressing the SELECTED attribute for non-aria-role table cells?

Correct, an accessible that is SELECTED must have a SELECTABLE state. So non-aria cells should net be SELECTED.

I'm tagging Jamie here just to make sure my assertions are correct. Jamie, non-aria table cells should never have implicit selectable/selected states, right?

Comment 15

[Andy]

It’s great to see that there’s some movement, thank you for taking this on.

Probably it’s just internal jargon, but to be sure: What is an aria-cell or a non-aria-cell?

The specification demands that if, and only if aria-selected is not empty, a cell should be SELECTABLE.

The option, tab, and treeitem roles permit user agents to provide an implicit value for aria-selected when specified conditions are met. User agents MUST NOT provide an implicit value for aria-selected in any other circumstance.

So the logic would be independent of any other aria atributes, right?

Value Description
false The selectable element is not selected.
true The selectable element is selected.
undefined (default) The element is not selectable.

From https://w3c.github.io/aria/#aria-selected

Comment 16

[briannapatricia7]

hello everyone, I am Brianna an outreachy applicant, i am a beginner in front end. but if i am guided , I can solve this issue
thanks

Comment 17

[Francis]

(In reply to bahatibrianp from comment #16)

hello everyone, I am Brianna an outreachy applicant, i am a beginner in front end. but if i am guided , I can solve this issue
thanks

Hey Brianna, I'm still working on this bug but right now I'm ruminating over some of the failing test cases from also removing NativeState (I have a few commitments over the side). I'm currently at a point where I'm close to sending in my first patch. Either way, you may want to ask Eitan (:eeejay) for guidance instead of Andy as he is the mentor for this bug.

Comment 18

[Andy]

Thank you Brianna for wanting to help with this, that's great.

Unfortunately I cannot support any guidance on how to approach this, I could help with understanding the specifications of explaining the use case. Don't hesitate to ask.

Good luck

Comment 19

[Francis]

After removing SELECTED and SELECTABLE states for non-aria-role table cells, test_sels_table.html of the mochitest-a11y suite is failing, but only because none of the non-aria-role table cells/rows/columns can ever have the SELECTED state. This is also the case for the test_table_1.html test case. Still, as these tests seem to be important, should they be replaced with equivalent tests that have aria-role table cells (e.g. by having <td> with aria-selected="false")? Also, is it worth to add a test to verify the absence of SELECTED/SELECTABLE states for non-aria-role table cells?

Comment 20

[Francis]

Attachment: WIP: Bug 1730474 - Restrict SELECTABLE state to only aria-role table cells. r?eeejay

Comment 21

[Francis]

Attachment: WIP: Bug 1730474 - Refactor to avoid return statement before else. r?eeejay

Depends on D174438

Comment 22

[Eitan Isaacson [:eeejay]]

Thanks for this! Those two patches should be folded into one. Note that testing for HasStrongARIARole is not enough because:

1. It just means the td has an aria role, not necessarily a grid cell.
2. If the table element has a role of grid its cells are implicitly grid cells even though they don't directly have an aria role defined, see here.

So the real check you want to do is probably Role() == roles::GRID_CELL.

Comment 23

[Francis]

Attachment: Bug 1730474 - Restrict SELECTABLE state to only aria-role table cells. r?eeejay

Comment 24

[Francis]

Attachment: Bug 1730474 - Restrict SELECTABLE state to only accessibles with gridcell role. r?eeejay

Depends on D174438

Comment 25

[Francis]

I've folded the two commits and submitted new changes with the suggested check to Phabricator as requested.

I ran the mochitest-a11y suite afterward, and the following tests had failures:

• test_sels_table: many failures. Adding role="grid" to the 'table' elements makes all but 3 test failures disappear. The 3 remaining failures appear to correspond to each of the 'th' nodes present in the page.
• test_table_1: 3 failures. When role="grid" is added to the 'table' element, only 2 failures, but this seems to be because the rowheader/columnheader accessibles don't have SELECTABLE state.

Please correct me if I'm wrong, but it seems that the next step is to ensure that rowheader/columnheader accessibles as conditionally having SELECTABLE state when within a table accessible with role="grid". I am unsure on how to approach this, so I was wondering whether you are able to provide pointers for this?

Comment 26

[Eitan Isaacson [:eeejay]]

(In reply to Francis from comment #25)

I've folded the two commits and submitted new changes with the suggested check to Phabricator as requested.

I ran the mochitest-a11y suite afterward, and the following tests had failures:

• test_sels_table: many failures. Adding role="grid" to the 'table' elements makes all but 3 test failures disappear. The 3 remaining failures appear to correspond to each of the 'th' nodes present in the page.
• test_table_1: 3 failures. When role="grid" is added to the 'table' element, only 2 failures, but this seems to be because the rowheader/columnheader accessibles don't have SELECTABLE state.

These tests should not be changed by adding role=grid, rather they should be changed to expect SELECTABLE/SELECTED to not be present in HTML tables.

Please correct me if I'm wrong, but it seems that the next step is to ensure that rowheader/columnheader accessibles as conditionally having SELECTABLE state when within a table accessible with role="grid". I am unsure on how to approach this, so I was wondering whether you are able to provide pointers for this?

Good point on row/column headers. I didn't think of that. A simple role check won't work there. You will need to do something similar this check to see if the containing table has a grid role.

Comment 27

[Francis]

Attachment: Bug 1730474 - Restrict SELECTABLE state of rowheader/columnheader accessibles to when only in grid. r?eeejay

Depends on D175615

Comment 28

[Francis]

Attachment: Bug 1730474 - Change tests to check that non-aria-role tables cannot be SELECTABLE. r?eeejay

Depends on D180319

Comment 29

[Eitan Isaacson [:eeejay]]

Looks like you got several dependent patches here that just alter the previous patch. Squash all the patches into a single one, abandon/delete the others and request review. Thanks!!

Comment 30

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

The following patches are waiting for review from a reviewer who resigned from the review:

ID	Title	Author	Reviewer Status
D175615	Bug 1730474 - Restrict SELECTABLE state to only accessibles with gridcell role. r?eeejay	rainyworlds1	eeejay: Resigned from review
D180320	Bug 1730474 - Change tests to check that non-aria-role tables cannot be SELECTABLE. r?eeejay	rainyworlds1	eeejay: Resigned from review

:rainyworlds1, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 31

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

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 32

[Olumide]

Hello Eitan, I am an Outreachy intern and I would love to work on this issue. Is it fine if I can take this up?

Comment 33

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

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.

Comment 34

[Eitan Isaacson [:eeejay]]

Looks like this has stalled.

Comment 35

[ishmam.mahmud]

Attachment: Bug 1730474 - Adding a test for the bug. r?eeejay

This patch only adds my first attempt at trying to reproduce the bug in
a test.

However, the test is failing with the very first ok() call right now,
and I need some guidance on whether I'm going at the right direction
with test.

Comment 36

[ishmam.mahmud]

I pushed the above patch https://phabricator.services.mozilla.com/D232604 which only includes a test. I'm trying to reproduce it in tests first before I start on a solution, but I'm not sure I'm doing things right. I mostly just copied ideas from the other tests in the same folder.
Can you take a look Eitan? I added you as a reviewer. If this direction makes sense I want to go further in solving this bug.

Comment 37

[Andy]

Thanks for looking into this and good luck with your endeavour (: