Remove role="label" aria processing.

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: davidb, Assigned: capella)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bolterbugz@gmail.com])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
No longer in spec (which seems crazy).

Updated

7 years ago
Whiteboard: [good first bug]

Updated

7 years ago
Duplicate of this bug: 634322
(Reporter)

Comment 2

7 years ago
I wonder if we want to pref this off or rip it out. I worry ARIA 2.0
might add it back.
Isn't that where revision control becomes handy?
(Reporter)

Updated

6 years ago
Assignee: bolterbugz → nobody
Whiteboard: [good first bug] → [good first bug][mentor=bolterbugz@gmail.com]
(Reporter)

Comment 4

6 years ago
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.

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
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

6 years ago
(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

6 years ago
Created attachment 592681 [details] [diff] [review]
The role="lable" has been removed
Attachment #592681 - Flags: review?(bolterbugz)
(Assignee)

Comment 8

6 years ago
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
(Reporter)

Comment 9

6 years ago
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+.
Attachment #592681 - Flags: review?(bolterbugz)
(Assignee)

Comment 10

6 years ago
Looks like I got lost in the shuffle ... someone reassign this to <pallavi> if that's the plan please.?
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Ok, thanks. I'll stay with it :-P
(Reporter)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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
Attachment #592711 - Flags: review?(bolterbugz)
(Reporter)

Comment 15

6 years ago
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?
(Assignee)

Comment 16

6 years ago
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
Attachment #592751 - Flags: review?(bolterbugz)
(Reporter)

Comment 17

6 years ago
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
(Assignee)

Comment 18

6 years ago
Created attachment 592768 [details] [diff] [review]
Try number three ... "label" vars changed

label VARs changed, label1 text and label2 text still shows on screen
Attachment #592768 - Flags: review?(bolterbugz)
(Reporter)

Comment 19

6 years ago
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!
Attachment #592768 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 20

6 years ago
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 ...
Attachment #592768 - Flags: review?(surkov.alexander)
Attachment #592768 - Flags: review?(bolterbugz)
Attachment #592768 - Flags: review+
(Reporter)

Updated

6 years ago
Attachment #592681 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #592711 - Attachment is obsolete: true
Attachment #592711 - Flags: review?(bolterbugz)
(Reporter)

Updated

6 years ago
Attachment #592751 - Attachment is obsolete: true
Attachment #592751 - Flags: review?(bolterbugz)
(Reporter)

Comment 21

6 years ago
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 :) )
Attachment #592768 - Flags: review?(bolterbugz) → review+

Comment 22

6 years ago
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
Attachment #592768 - Flags: review?(surkov.alexander) → review-
(Reporter)

Comment 23

6 years ago
(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.
(Assignee)

Comment 24

6 years ago
Created attachment 593076 [details] [diff] [review]
592768: Try number four ... various code cleanup
Attachment #593076 - Flags: review?(surkov.alexander)
(Assignee)

Comment 25

6 years ago
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

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

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

r=me, thanks.
Attachment #593076 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 27

6 years ago
Can you briefly explain how easily to add the r=you (:surkov)
(Assignee)

Comment 28

6 years ago
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

6 years ago
(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

Updated

6 years ago
Attachment #592768 - Attachment is obsolete: true
(Assignee)

Comment 30

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

Comment 31

6 years ago
(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

6 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/637a6693195b
https://hg.mozilla.org/mozilla-central/rev/637a6693195b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.