782544 - Don't expose position info for table cells

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Attachment: Test case.

Currently, Gecko exposes group position info (e.g. IAccessible2::groupPosition and posinset and setsize attributes) on certain table cells, such as ARIA gridcells and XUL tree tables. However, it doesn't for real HTML table cells using the td tag. Exposing group position info on table cells is redundant, since the table cell coordinates (e.g. available via IAccessibleTableCell) provide more information anyway. Therefore, I believe that all table cells should not expose group position info.

Str:
1. Open the attached test case.
2. Retrieve the accessible of a table cell in the first table.
3. Check its exposed group position info.
Result (correct): There is no group position info.
4. Retrieve the accessible of a table cell in the second table.
5. Check its exposed group position info.
Expected: There should be no group position info.
Actual: There is group position info.

Comment 1

[James Teh [:Jamie]]

Real world impact: This causes NVDA to be overly verbose when reading these table cells; e.g. "Update on poster 4 of 8 Subject column 4" instead of just "Update on poster Subject column 4".

Comment 2

[alexander :surkov (:asurkov)]

It seems ARIA was changed, aria-level and etc aren't longer applied to gridcells (http://www.w3.org/TR/wai-aria/states_and_properties#aria-level). You need to fix AccGroupInfo.h/cpp and remove logic for CELL roles.

Note, you need to fix test_obj_group.html (related with ARIA grid and treegrid) and add tests into test_obj_group_tree.xul (missed tests for XUL tree).

Comment 3

[rajesh kumar]

Hey, can I be assigned this bug???

Comment 4

[alexander :surkov (:asurkov)]

(In reply to rajesh kumar from comment #3)
> Hey, can I be assigned this bug???

sure, thank you for taking it

Comment 5

[rajesh kumar]

Can you please help me in how to go about this since I am new to this.
Thanks a lot

Comment 6

[alexander :surkov (:asurkov)]

(In reply to rajesh kumar from comment #5)
> Can you please help me in how to go about this since I am new to this.
> Thanks a lot

see comment #2, locate these files and get rid CELL role entries. please be more specific in questions - I don't know how much new you are :)

Comment 7

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 8

[Kamil Muszynski (:kamilm)]

Hi,

I know this bug is a little dated, but does it still need fixing?

Regards,
Kamil

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Kamil Muszyński (:kamilm) from comment #8)
> I know this bug is a little dated, but does it still need fixing?

yes, please feel free to take it

Comment 10

[mjh563]

Kamil, do you want to work on fixing this bug? If so, someone can assign it to you.

Comment 11

[Kamil Muszynski (:kamilm)]

Yes I do. Please assign it to me if you can.

Comment 12

[Kamil Muszynski (:kamilm)]

OK, 
so far I've removed all the code related to GRID_CELL in AccGroupInfo.h/cpp files, as Alexander suggested. I've checked the accessible properties using DOM inspector and now the span items from the test case attached (with role='gridcell') don't have setsize and posinset properties anymore - is that the correct behavior? Sorry for basic questions, ARIA is something totally new for me.

Comment 13

[alexander :surkov (:asurkov)]

yes, did you run mochitests?

Comment 14

[Kamil Muszynski (:kamilm)]

Hi, 
sorry for late reply. I've managed to run mochitests (make -C obj-dbg mochitest-a11y) - my modification causes some tests from test_obj_group.html to fail. I'll try to figure out how to rewrite them.

Comment 15

[Kamil Muszynski (:kamilm)]

Attachment: patch.diff

First proposal of a patch.

Comment 16

[Kamil Muszynski (:kamilm)]

Alexander, should I put you as a reviewer for this patch?

I've attached changes to AccGroupInfo.h/cpp and test_obj_group.html. I don't know how to test accessibility informations for test_obj_group_tree.xul. For HTML file I used FF Nightly build with DOM Inspector addon, where I can check if posinset and setsize attributes were present. Any tips how can I do similar thing with XUL file? I'm unable to open it in Firefox, as this file format is no longer supported for security issues.

Thanks in advance.

Comment 17

[mjh563]

You can open local XUL files by setting dom.allow_XUL_XBL_for_file to true in about:config (create the pref if it doesn't already exist).

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 823019 [details] [diff] [review]
patch.diff

Review of attachment 823019 [details] [diff] [review]:
-----------------------------------------------------------------

otherwise it looks ok

::: accessible/src/base/AccGroupInfo.cpp
@@ +213,4 @@
>    if ((aParentRole == roles::TABLE || aParentRole == roles::TREE_TABLE) &&
>        aRole == roles::ROW)
>      return true;
> +  if (aParentRole == roles::ROW && aRole == roles::CELL)

I'd say it either should be removed entirely or kept entirely. Do you any use case for it?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Kamil Muszyński (:kamilm) from comment #16)
> Alexander, should I put you as a reviewer for this patch?
> 
> I've attached changes to AccGroupInfo.h/cpp and test_obj_group.html. I don't
> know how to test accessibility informations for test_obj_group_tree.xul.

you should do

mach mochitest-a11y --keep-open path-to-test-in-src-dir

Comment 20

[Kamil Muszynski (:kamilm)]

That's what I did - but mach mochitests-a11y runs FF without my extensions installed, so I don't have dom inspector available :) 

For the use case (review question) - probably not, I can remove this line entirely.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Kamil Muszyński (:kamilm) from comment #20)
> That's what I did - but mach mochitests-a11y runs FF without my extensions
> installed, so I don't have dom inspector available :) 

technically you can copy inspector from extensions folder to extensions folder of tests but if tests are green then it works.

> For the use case (review question) - probably not, I can remove this line
> entirely.

ok

Comment 22

[Kamil Muszynski (:kamilm)]

Attachment: patch.diff

Changes after the first review

Comment 23

[Kamil Muszynski (:kamilm)]

Alexander, just one more question - I'm not sure what accessibility properties should be assigned now to XUL tree items. When I check them using DomInspector (file: test_obj_group_tree.xul), posinset and setsize for tree nodes (row1col, row2_col, etc.) are still present, and the presence of those parameters is already checked in that test. Any tips?

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 826959 [details] [diff] [review]
patch.diff

Review of attachment 826959 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb9f8a3e326

Thanks for the patch, Kamil! One request, can you please make sure that future patches have all the necessary commit information included with them? The below link should give you the information you need to do so. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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