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.

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?

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 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?

>It sounds gridcell role is used in html:tables only to allow name computing >from subtree? That's what I understood, yes.

>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 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.

(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.

(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.

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.

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 on attachment 357784 [details] [diff] [review] Patch2 looks ok, I'll check whitespaces

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

but I forgot to add new files

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