474281 - accessible name of html table cells is incorrectly including descendants

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Curran]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090116 Minefield/3.2a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090116 Minefield/3.2a1pre ID:20090116035550

When a html table cell (<td>) contains content, this content all gets included in its accessible name.
This can become annoying especially in web applications as screen readers may read all the content as it is the name of the cell, when moving to one of its descendants with the focus.
I attach an html example which shows that the accessible name of the table cell contains all its textual content.
ARIA has rules in which its ok to include descendants in the name, but I feel for generic table cells (that are not focusable themselves) this should not happen.

Reproducible: Always

Steps to Reproduce:
1. Open the attached html example in Firefox 3.2
2. Using either NVDA's object navigation or Accessibility Probe, navigate to the single cell in the table.
3. Take note of its accessible name.
Actual Results:  
The accessible name contains the text of the paragraph, the link and the list, which are contained within the table cell.

Expected Results:  
The table cell should have an empty accessible name.

This bug does not seem to occure in Firefox 3.0. I am not sure about 3.1, but I am definit about it happening in 3.2a1.

Comment 1

[Michael Curran]

Attachment: test case showing that a table cell with content.

Comment 2

[alexander :surkov (:asurkov)]

the reason is bug 467139, "NameFromSubtree" rule should be based on accessible role. ARIA role "gridcell" (ROLE_CELL) allows to get the name from subtree and Marco said this is correct. HTML td (ROLE_CELL) shouldn't allow to get name. Aaron, can you comment this?

Comment 3

[Aaron Leventhal]

Ok, we need a separate nsIAccessible role then, for table cell vs. gridcell.

This probably already works alright for listitem vs. option -- I believe we use separate roles for those, even though the MSAA role is the same.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 357773 [details] [diff] [review]
Patch

>diff --git a/accessible/public/nsIAccessibleRole.idl b/accessible/public/nsIAccessibleRole.idl
>--- a/accessible/public/nsIAccessibleRole.idl
>+++ b/accessible/public/nsIAccessibleRole.idl
>@@ -39,17 +39,17 @@
> #include "nsISupports.idl"
> 
> /**
>  * Defines cross platform (Gecko) roles.
>  *
>  * @note - When adding a new role, be sure to also add it to nsRoleMap.h for
>  *         each platform.
>  */
>-[scriptable, uuid(8c0f68f8-164a-4078-a9ee-36a7d180f0e4)]
>+[scriptable, uuid(6793ca5c-c7cb-41db-9fb9-c16c0525f962)]
> interface nsIAccessibleRole : nsISupports
> {
>   /**
>    * Used when accessible hans't strong defined role.
>    */
>   const unsigned long ROLE_NOTHING = 0;
> 
>   /**
>@@ -225,18 +225,18 @@ interface nsIAccessibleRole : nsISupport
>   const unsigned long ROLE_COLUMN = 27;
> 
>   /**
>    * Represents a row of cells within a table. Also, see ROLE_TABLE.
>    */
>   const unsigned long ROLE_ROW = 28;
> 
>   /**
>-   * Represents a cell within a table. Is is used for html:td,
>-   * role="gridcell". Also, see ROLE_TABLE.
>+   * Represents a cell within a table. Is is used for html:td.
>+   * Also, see ROLE_TABLE.

Also it is used for xul:tree cell and listcell.

>   /**
>-   * It's not role actually. This contanst is important to help ensure
>+   * Represents a cell within a grid. It is used for role="gridcell".
>+   * Also, see ROLE_TABLE.

Would be nice to comment here difference between ROLE_CELL and this role.

>diff --git a/accessible/tests/mochitest/Makefile.in b/accessible/tests/mochitest/Makefile.in
>--- a/accessible/tests/mochitest/Makefile.in
>+++ b/accessible/tests/mochitest/Makefile.in
>@@ -49,16 +49,17 @@ _TEST_FILES =\
> 		moz.png \
> 		longdesc_src.html \
> 		attributes.js \
> 		common.js \
> 		nsIAccessible_actions.js \
> 		nsIAccessible_name.css \
> 		nsIAccessible_name.js \
> 		nsIAccessible_name.xbl \
>+		nsIAccessible_role.js \

I would prefer to call it role.js like we do for attributes.js and common.js (it would be nice to keep names simpler)

>  		nsIAccessible_selects.js \
> 		nsIAccessible_states.js \
> 		nsIAccessibleEditableText.js \
> 		test_aria_activedescendant.html \
> 		test_aria_role_article.html \
> 		test_aria_role_equation.html \
> 		test_aria_token_attrs.html \
> 		$(warning test_bug368835.xul temporarily disabled) \
>@@ -91,16 +92,17 @@ _TEST_FILES =\
> 		test_nsIAccessibleImage.html \
> 		test_nsIAccessibleTable_1.html \
> 		test_nsIAccessibleTable_2.html \
> 		test_nsIAccessibleTable_3.html \
> 		test_nsIAccessibleTable_4.html \
> 		$(warning test_nsIAccessibleTable_listboxes.xul temporarily disabled) \
> 		test_nsIAccessNode_utils.html \
> 		test_nsOuterDocAccessible.html \
>+		test_table_and_grid.html \

would it be good to keep test_role.html file name and rename test_nsHyperTextAcc_roles. to test_roles_nsHyperTextAcc?

> 		test_textattrs.html \
> 		test_textboxes.html \
> 		test_textboxes.xul \
> 		testTextboxes.js \
> 		test_bug429285.html \
> 		test_bug434464.html \
> 		$(NULL)
> 
>diff --git a/accessible/tests/mochitest/common.js b/accessible/tests/mochitest/common.js
>--- a/accessible/tests/mochitest/common.js
>+++ b/accessible/tests/mochitest/common.js
>@@ -35,16 +35,17 @@ const nsIObserverService = Components.in
> 
> const nsIDOMNode = Components.interfaces.nsIDOMNode;
> const nsIPropertyElement = Components.interfaces.nsIPropertyElement;
> 
> ////////////////////////////////////////////////////////////////////////////////
> // Roles
> 
> const ROLE_PUSHBUTTON = nsIAccessibleRole.ROLE_PUSHBUTTON;
>+const ROLE_CELL = nsIAccessibleRole.ROLE_CELL;

where is ROLE_GRID_CELL?

>+  <script type="application/javascript">
>+    function doTests()
>+    {
>+      // Test table cell name and role
>+      testName("tc", null);

would be nice to save name testing for bug 468034.

>+      testRole("tc", ROLE_CELL);
>+
>+      // Test grid cell.
>+      testName("gc", "This is a paragraph This is a link    This is a list");
>+      testRole("gc", ROLE_CELL);

why do they have the same role?

>+  <p id="display"></p>
>+  <div id="content" style="display: none"></div>
>+  <pre id="test">
>+  </pre>
>+  <table>
>+    <tr>
>+      <td id="tc">
>+        <p>This is a paragraph</p>
>+        <a href="#">This is a link</a>
>+        <ul>
>+          <li>This is a list</li>
>+        </ul>
>+      </td>
>+    </tr>
>+  </table>
>+  <table>
>+    <tr>
>+      <td id="gc" role="gridcell">

It sounds gridcell role is used in html:tables only to allow name computing from subtree?

Comment 6

[Marco Zehe (:MarcoZ)]

>It sounds gridcell role is used in html:tables only to allow name computing
>from subtree?

That's what I understood, yes.

Comment 7

[Marco Zehe (:MarcoZ)]

>would be nice to save name testing for bug 468034.

Well, the thought was to test the name now while checking in the patch, and then replace this with the name calculation once we have bug 468034 actually landed.

Since the main point of this bug is to fix the name calculation, I am thinking about removing the role testing entirely. To the outside world, there is no difference anyway, and even our internal API uses it only for the name calculation rule, but exposes it as a ROLE_CELL in any case. What do you think?

Comment 8

[Aaron Leventhal]

Comment on attachment 357773 [details] [diff] [review]
Patch

I did not look at the tests, but the rest of the patch looks exactly correct. There are a couple of whitespace nits that need to be addressed, so you might ask someone else to check it in for you with whitespace changes. Nice work.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> >would be nice to save name testing for bug 468034.
> 
> Well, the thought was to test the name now while checking in the patch, and
> then replace this with the name calculation once we have bug 468034 actually
> landed.

Fine with me. Just XXX sections would be good to remember.

> Since the main point of this bug is to fix the name calculation, I am thinking
> about removing the role testing entirely.

I do not see anything bad with role testing. It may be good think, additional protection and so on.

> To the outside world, there is no
> difference anyway, and even our internal API uses it only for the name
> calculation rule, but exposes it as a ROLE_CELL in any case. What do you think?

No, actually, I believe it exposes ROLE_GRID_CELL role, I just missed error in your mochitest

    is(acc.role, aRole, "Wrong role for " + aID + "!");

you should test finalRole actually. I think "role" will be  removed eventually from scriptable interface.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (From update of attachment 357773 [details] [diff] [review])

> There are a couple of whitespace nits that need to be addressed, so you might
> ask someone else to check it in for you with whitespace changes. Nice work.

If there are wrong white space then I can fix and file new patch.

Comment 11

[Marco Zehe (:MarcoZ)]

I'll put up a new patch soon, just running a build to check that I got all the renames and Makefile entries right. This patch will then hopefully address all nits.

Comment 12

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

1. Now properly tests for ROLE_GRID_CELL.
2. Hopefully fixed whitespace nits. I found one in nsNameUtil.cpp.
3. Hopefully addressed all other comments.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 357784 [details] [diff] [review]
Patch2

looks ok, I'll check whitespaces

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch - few nits

mostly all is ok with whitespaces, I fixed two nits.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: with new files

but I forgot to add new files

Comment 16

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/rev/f177a1157d4b
with Surkov's whitespace fixes.