Don't expose position info for table cells

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Jamie, Assigned: kamilm)

Tracking

Trunk
mozilla28
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

Posted file 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.
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".
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).
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
Hey, can I be assigned this bug???
(In reply to rajesh kumar from comment #3)
> Hey, can I be assigned this bug???

sure, thank you for taking it
Assignee: nobody → mailto.rajesh05
Can you please help me in how to go about this since I am new to this.
Thanks a lot
(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 :)
Can you confirm that you're still working on this bug?
Flags: needinfo?(mailto.rajesh05)
Hi,

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

Regards,
Kamil
(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
Kamil, do you want to work on fixing this bug? If so, someone can assign it to you.
Assignee: mailto.rajesh05 → nobody
Flags: needinfo?(mailto.rajesh05)
Yes I do. Please assign it to me if you can.
Assignee: nobody → muszynski.kamil
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.
yes, did you run mochitests?
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.
Posted patch patch.diff (obsolete) — Splinter Review
First proposal of a patch.
Attachment #823019 - Flags: review?
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.
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 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?
Attachment #823019 - Flags: review? → review+
(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
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.
(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
Posted patch patch.diffSplinter Review
Changes after the first review
Attachment #823019 - Attachment is obsolete: true
Attachment #826959 - Flags: review?
Attachment #826959 - Flags: review? → review?(surkov.alexander)
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 on attachment 826959 [details] [diff] [review]
patch.diff

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

r=me
Attachment #826959 - Flags: review?(surkov.alexander) → review+
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dfb9f8a3e326
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.