589784 - Remove role="label" aria processing.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

No longer in spec (which seems crazy).

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Isn't that where revision control becomes handy?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

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

On Wed, Jan 25, 2012 at 5:31 AM, Mark Capella <[e-mail]> 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 (:asurkov)]

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

Attachment: The role="lable" has been removed

Comment 8

[Mark Capella [:capella]]

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] (NeedInfo me for attention)]

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

Looks like I got lost in the shuffle ... someone reassign this to <pallavi> if that's the plan please.?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Ok, thanks. I'll stay with it :-P

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Attachment: 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] (NeedInfo me for attention)]

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

Attachment: New patch for Bug 589784

Added change for test_childAtPoint.html, combined changesets, scanned for more role="label" references

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Attachment: Try number three ... "label" vars changed

label VARs changed, label1 text and label2 text still shows on screen

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

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

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] (NeedInfo me for attention)]

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 (:asurkov)]

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] (NeedInfo me for attention)]

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

Attachment: 592768: Try number four ... various code cleanup

Comment 25

[Mark Capella [:capella]]

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 (:asurkov)]

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

Can you briefly explain how easily to add the r=you (:surkov)

Comment 28

[Mark Capella [:capella]]

Comment on attachment 593076 [details] [diff] [review]
592768: Try number four ... various code cleanup

># HG changeset patch
># Parent bfeeb813aef2dfe25a74343034664127d954b274
># User Mark Capella <[e-mail]>
>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 (:asurkov)]

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

Maybe not important, let me know what I can do next

Comment 31

[alexander :surkov (:asurkov)]

(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 (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/637a6693195b

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/637a6693195b