Last Comment Bug 589784 - Remove role="label" aria processing.
: Remove role="label" aria processing.
Status: RESOLVED FIXED
[good first bug][mentor=bolterbugz@gm...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
:
Mentors:
: 634322 (view as bug list)
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2010-08-23 07:50 PDT by David Bolter [:davidb]
Modified: 2012-02-02 07:14 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The role="lable" has been removed (425 bytes, patch)
2012-01-30 05:59 PST, pallavi
no flags Details | Diff | Splinter Review
Patch and Test for Bug 589784 (1.62 KB, patch)
2012-01-30 07:36 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
New patch for Bug 589784 (3.30 KB, patch)
2012-01-30 09:26 PST, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Try number three ... "label" vars changed (4.04 KB, patch)
2012-01-30 10:17 PST, Mark Capella [:capella]
dbolter: review+
surkov.alexander: review-
Details | Diff | Splinter Review
592768: Try number four ... various code cleanup (4.03 KB, patch)
2012-01-31 06:30 PST, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description David Bolter [:davidb] 2010-08-23 07:50:29 PDT
No longer in spec (which seems crazy).
Comment 1 alexander :surkov 2011-03-10 07:40:19 PST
*** Bug 634322 has been marked as a duplicate of this bug. ***
Comment 2 David Bolter [:davidb] 2011-03-10 07:41:47 PST
I wonder if we want to pref this off or rip it out. I worry ARIA 2.0
might add it back.
Comment 3 Eitan Isaacson [:eeejay] 2011-11-01 08:11:04 PDT
Isn't that where revision control becomes handy?
Comment 4 David Bolter [:davidb] 2012-01-23 12:45:26 PST
Mentor note: this bug is about the "label" role as defined in accessible/src/base/nsARIAMap.cpp

The reason to remove this is that it isn't in spec. Put another way, there is currently (at the time I write this), no "label" role defined, and www.w3.org/TR/wai-aria/roles#label doesn't resolve.
Comment 5 Mark Capella [:capella] 2012-01-25 22:20:31 PST
On Wed, Jan 25, 2012 at 5:31 AM, Mark Capella <markcapella@twcny.rr.com> wrote:
      The bug says it's a good first bug ...

      Is there more to this than removing the nsRoleMapEntry for "label" from the source "rip it out"?

     {
       "label",
       roles::LABEL,
       kUseMapRole,
       eNoValue,
       eNoAction,
       eNoLiveAttr,
       kNoReqStates
     },

On 1/25/2012 1:42 PM, David Bolter wrote:
> That might be all there is to it. I would try that and then run our test suite.
>
> There is info on running the automated tests here:
> https://wiki.mozilla.org/Accessibility/Contribute
>
> Namely: make -C objdir mochitest-a11y
> (Don't interact with your keyboard/mouse while tests run)
>
> There might be some tests to remove/fix.
>

(The above attached for background)

   ) Okay, running the test....

        )) Before removing LABEL role    Passed:    21408,    Failed: 0,    ToDo:    1166
        )) After removing LABEL role       Passed:    21407,    Failed: 1,    ToDo:    1166

            Failing Test:        TEST_ARIA_ROLES.HTML
                                        Test URL | Wrong Role for 'cell' got 29 expected 0

            Code involved:    testRole("cell", ROLE_NOTHING);

                                          <!-- test gEmptyRoleMap -->
                                          <table role="label">
                                            <tr>
                                              <td id="cell">cell</td>
                                            </tr>
                                          </table>

    ) Changing Test Code to:        // testRole("cell", ROLE_NOTHING);                <-- comment out

        )) Produces                                   Passed:    21407,    Failed: 0,    ToDo:    1166        <-- one less test performed
Comment 6 alexander :surkov 2012-01-25 22:32:19 PST
(In reply to Mark Capella from comment #5)
>                                           <!-- test gEmptyRoleMap -->
>                                           <table role="label">
>                                             <tr>
>                                               <td id="cell">cell</td>
>                                             </tr>
>                                           </table>
> 
>     ) Changing Test Code to:        // testRole("cell", ROLE_NOTHING);      
> <-- comment out

general practice to use mxr (hg history) to see what this test was about.

gEmptyRoleMap is Empty role map entry. Used by accessibility service to create an accessible if the accessible can't use role of used accessible class. For example, it is used for table cells that aren't contained by table.

that means you can fix this test by putting any other role having strong accessible role like role="button" for example.
Comment 7 pallavi 2012-01-30 05:59:15 PST
Created attachment 592681 [details] [diff] [review]
The role="lable" has been removed
Comment 8 Mark Capella [:capella] 2012-01-30 06:32:50 PST
Changing to role=button allows test to work fine ... today I tried changing it to role=img, another strong role with a gWAIRoleMap that more closely resembles role=label ...

Test works fine there also, but now TEST_DOCLOAD.XUL fails for img and button with a bug resembling Bug 691005 - Intermittent test_docload.xul | Doubled event ...

checking ... ooops just spotted comment 7
Comment 9 David Bolter [:davidb] 2012-01-30 06:35:52 PST
Comment on attachment 592681 [details] [diff] [review]
The role="lable" has been removed

Thanks. That's the right start.

So I ran tests and got one failure that will need to be fixed. It is the test case for the cell inside <table role="label"> in test_aria_roles.html.

There is another place we use role="label" in our tests. It doesn't fail but we should clean that up as well.

We have some information on running tests here:
https://wiki.mozilla.org/Accessibility/Contribute

Almost there. I'll remove the review request on this patch since the tests will need to pass before I can r+.
Comment 10 Mark Capella [:capella] 2012-01-30 06:42:06 PST
Looks like I got lost in the shuffle ... someone reassign this to <pallavi> if that's the plan please.?
Comment 11 David Bolter [:davidb] 2012-01-30 07:06:57 PST
I jumped right to the patch review and didn't notice it wasn't from you Mark.

Mark this is your bug. I think you are further along and it is unusal for someone else to drop a drive-by patch, especially that has already been spelled out by yourself in comment 5.

Pallavi, thanks for the patch and please find yourself a different unassigned bug.
Comment 12 Mark Capella [:capella] 2012-01-30 07:10:44 PST
Ok, thanks. I'll stay with it :-P
Comment 13 David Bolter [:davidb] 2012-01-30 07:14:40 PST
Cool.

(In reply to Mark Capella from comment #8)
> Changing to role=button allows test to work fine ... today I tried changing
> it to role=img, another strong role with a gWAIRoleMap that more closely
> resembles role=label ...

I think button is fine. You point isn't to make the role close to label, just that the role overrides the semantic of table (i.e. the role is not a landmark role).

> Test works fine there also, but now TEST_DOCLOAD.XUL fails for img and
> button with a bug resembling Bug 691005 - Intermittent test_docload.xul |
> Doubled event ...

Ignore that failure if it is intermittent.

Happy to review a patch when you're ready.
Comment 14 Mark Capella [:capella] 2012-01-30 07:36:59 PST
Created attachment 592711 [details] [diff] [review]
Patch and Test for Bug 589784

Here she is ... ignoring intermittent bug from apparantly un-related test as mentioned previously
Comment 15 David Bolter [:davidb] 2012-01-30 07:39:21 PST
Comment on attachment 592711 [details] [diff] [review]
Patch and Test for Bug 589784

Looks great. Can you clean up the role="label" in test_childAtPoint.html as well?
Comment 16 Mark Capella [:capella] 2012-01-30 09:26:17 PST
Created attachment 592751 [details] [diff] [review]
New patch for Bug 589784

Added change for test_childAtPoint.html, combined changesets, scanned for more role="label" references
Comment 17 David Bolter [:davidb] 2012-01-30 09:34:21 PST
Comment on attachment 592751 [details] [diff] [review]
New patch for Bug 589784

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

Cheering you on here, and it gets more interesting:

::: accessible/tests/mochitest/test_childAtPoint.html
@@ +42,3 @@
>        var labelText = label.firstChild;
>        testChildAtPoint(label, 1, 1, false, labelText);
>        testChildAtPoint(label, 1, 1, true, labelText);

The variable names are no incorrect/misleading. Can you figure out what this was testing and rework it?

It was introduced here:
http://hg.mozilla.org/mozilla-central/rev/ff250122fa99
Comment 18 Mark Capella [:capella] 2012-01-30 10:17:47 PST
Created attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

label VARs changed, label1 text and label2 text still shows on screen
Comment 19 David Bolter [:davidb] 2012-01-30 11:42:06 PST
Comment on attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

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

r=me, but please have Alexander Surkov take a look at the test_childAtPoint.html change.

::: accessible/tests/mochitest/test_childAtPoint.html
@@ +40,5 @@
> +      // Not specific case, point is inside of btn accessible.
> +      var bttn = getAccessible("btn");
> +      var bttnText = bttn.firstChild;
> +      testChildAtPoint(bttn, 1, 1, false, bttnText);
> +      testChildAtPoint(bttn, 1, 1, true, bttnText);

Please add a review request for :surkov to review this change. Thanks!
Comment 20 Mark Capella [:capella] 2012-01-30 11:59:58 PST
Comment on attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

Unfamiliar with the mechanics, but ... Trying to request review from David, and :surkov (Alexander Surkov) also ...
Comment 21 David Bolter [:davidb] 2012-01-30 12:40:51 PST
Comment on attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

Redoing my r+ :)
Mark, for next time, there is a combo box at the bottom for 'addl. review'. You can just add an additional review there. (But don't do that now :) )
Comment 22 alexander :surkov 2012-01-30 23:12:10 PST
Comment on attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

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

::: accessible/src/base/nsARIAMap.cpp
@@ -208,5 @@
> -    eNoValue,
> -    eNoAction,
> -    eNoLiveAttr,
> -    kNoReqStates
> -  },

can you say me please why role="heading" is guilty?

::: accessible/tests/mochitest/test_childAtPoint.html
@@ +37,5 @@
>        testChildAtPoint(txt, -10000, 10000, false, null);
>        testChildAtPoint(txt, -10000, 10000, true, null);
>  
> +      // Not specific case, point is inside of btn accessible.
> +      var bttn = getAccessible("btn");

it seems to me this variable stutters, any reason why 'btn' doesn't work?

@@ +77,5 @@
>    <div role="list" id="list">
>      <div role="listitem" id="listitem"><span role="image" id="image">img</span>item</div>
>    </div>
>  
> +  <span role="button">label1</span><span role="button" id="btn">label2</span>

it makes sense to change text too
Comment 23 David Bolter [:davidb] 2012-01-31 06:08:00 PST
(In reply to alexander :surkov from comment #22)
> Comment on attachment 592768 [details] [diff] [review]
> Try number three ... "label" vars changed
> 
> Review of attachment 592768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsARIAMap.cpp
> @@ -208,5 @@
> > -    eNoValue,
> > -    eNoAction,
> > -    eNoLiveAttr,
> > -    kNoReqStates
> > -  },
> 
> can you say me please why role="heading" is guilty?

Ha! I stopped checking that after the first patch. This would surely fail tests.
Comment 24 Mark Capella [:capella] 2012-01-31 06:30:20 PST
Created attachment 593076 [details] [diff] [review]
592768: Try number four ... various code cleanup
Comment 25 Mark Capella [:capella] 2012-01-31 06:32:15 PST
Fixed the Label1 and Label2 text, cleaned up bttn to be btn... HEADING role had dissapeared due to accidental source deletion and re-creation
Comment 26 alexander :surkov 2012-01-31 06:35:25 PST
Comment on attachment 593076 [details] [diff] [review]
592768: Try number four ... various code cleanup

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

r=me, thanks.
Comment 27 Mark Capella [:capella] 2012-01-31 06:55:53 PST
Can you briefly explain how easily to add the r=you (:surkov)
Comment 28 Mark Capella [:capella] 2012-01-31 08:48:50 PST
Comment on attachment 593076 [details] [diff] [review]
592768: Try number four ... various code cleanup

># HG changeset patch
># Parent bfeeb813aef2dfe25a74343034664127d954b274
># User Mark Capella <markcapella@twcny.rr.com>
>Bug 589784 - Remove role="label" aria processing , No longer in spec; r=davidb, surkov
>
>
>diff --git a/accessible/src/base/nsARIAMap.cpp b/accessible/src/base/nsARIAMap.cpp
>--- a/accessible/src/base/nsARIAMap.cpp
>+++ b/accessible/src/base/nsARIAMap.cpp
>@@ -215,25 +215,16 @@ nsRoleMapEntry nsARIAMap::gWAIRoleMap[] 
>     roles::GRAPHIC,
>     kUseMapRole,
>     eNoValue,
>     eNoAction,
>     eNoLiveAttr,
>     kNoReqStates
>   },
>   {
>-    "label",
>-    roles::LABEL,
>-    kUseMapRole,
>-    eNoValue,
>-    eNoAction,
>-    eNoLiveAttr,
>-    kNoReqStates
>-  },
>-  {
>     "link",
>     roles::LINK,
>     kUseMapRole,
>     eNoValue,
>     eJumpAction,
>     eNoLiveAttr,
>     states::LINKED
>   },
>diff --git a/accessible/tests/mochitest/test_aria_roles.html b/accessible/tests/mochitest/test_aria_roles.html
>--- a/accessible/tests/mochitest/test_aria_roles.html
>+++ b/accessible/tests/mochitest/test_aria_roles.html
>@@ -126,17 +126,17 @@ https://bugzilla.mozilla.org/show_bug.cg
>   <table role="main" id="main_table">main table
>     <tr><td>hi<td></tr></table>
>   <table role="navigation" id="navigation_table">navigation table
>     <tr><td>hi<td></tr></table>
>   <table role="search" id="search_table">search table
>     <tr><td>hi<td></tr></table>
> 
>   <!-- test gEmptyRoleMap -->
>-  <table role="label">
>+  <table role="button">
>     <tr>
>       <td id="cell">cell</td>
>     </tr>
>   </table>
> 
>   <!-- user agents must not map abstract roles to platform API -->
>   <!-- test abstract base type roles -->
>   <div role="composite" id="composite">composite</div>
>diff --git a/accessible/tests/mochitest/test_childAtPoint.html b/accessible/tests/mochitest/test_childAtPoint.html
>--- a/accessible/tests/mochitest/test_childAtPoint.html
>+++ b/accessible/tests/mochitest/test_childAtPoint.html
>@@ -32,25 +32,25 @@
>       // document.
>       testChildAtPoint(txt, -1, 1, false, null);
>       testChildAtPoint(txt, -1, 1, true, null);
> 
>       // ::MustPrune case, point is outside of root accessible.
>       testChildAtPoint(txt, -10000, 10000, false, null);
>       testChildAtPoint(txt, -10000, 10000, true, null);
> 
>-      // Not specific case, point is inside of label accessible.
>-      var label = getAccessible("label");
>-      var labelText = label.firstChild;
>-      testChildAtPoint(label, 1, 1, false, labelText);
>-      testChildAtPoint(label, 1, 1, true, labelText);
>+      // Not specific case, point is inside of btn accessible.
>+      var btn = getAccessible("btn");
>+      var btnText = btn.firstChild;
>+      testChildAtPoint(btn, 1, 1, false, btnText);
>+      testChildAtPoint(btn, 1, 1, true, btnText);
>   
>-      // Not specific case, point is outside of label accessible.
>-      testChildAtPoint(label, -1, 1, false, null);
>-      testChildAtPoint(label, -1, 1, true, null);
>+      // Not specific case, point is outside of btn accessible.
>+      testChildAtPoint(btn, -1, 1, false, null);
>+      testChildAtPoint(btn, -1, 1, true, null);
> 
>       // Out of flow accessible testing, do not return out of flow accessible
>       // because it's not a child of the accessible even visually it is.
>       var rectArea = getNode("area").getBoundingClientRect();
>       var outOfFlow = getNode("outofflow");
>       outOfFlow.style.left = rectArea.left + "px";
>       outOfFlow.style.top = rectArea.top + "px";
> 
>@@ -73,17 +73,17 @@
>   <div id="content" style="display: none"></div>
>   <pre id="test">
>   </pre>
> 
>   <div role="list" id="list">
>     <div role="listitem" id="listitem"><span role="image" id="image">img</span>item</div>
>   </div>
> 
>-  <span role="label">label1</span><span role="label" id="label">label2</span>
>+  <span role="button">button1</span><span role="button" id="btn">button2</span>
> 
>   <span role="textbox">textbox1</span><span role="textbox" id="txt">textbox2</span>
> 
>   <div id="outofflow" style="width: 10px; height: 10px; position: absolute; left: 0px; top: 0px; background-color: yellow;">
>   </div>
>   <div id="area" style="width: 100px; height: 100px; background-color: blue;"></div>
> 
> </body>
Comment 29 alexander :surkov 2012-01-31 17:50:23 PST
(In reply to Mark Capella from comment #27)
> Can you briefly explain how easily to add the r=you (:surkov)

not sure what you are asking
Comment 30 Mark Capella [:capella] 2012-02-01 04:43:47 PST
Maybe not important, let me know what I can do next
Comment 31 alexander :surkov 2012-02-01 17:00:26 PST
(In reply to Mark Capella from comment #30)
> Maybe not important, let me know what I can do next

I'll land your patch. If you are looking what you can work next then you might want to look at bug 689540
Comment 32 alexander :surkov 2012-02-01 17:07:13 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/637a6693195b
Comment 33 Ed Morley [:emorley] 2012-02-02 07:14:22 PST
https://hg.mozilla.org/mozilla-central/rev/637a6693195b

Note You need to log in before you can comment on or make changes to this bug.