481114 - map timer, log and marquee ARIA roles into a11y roles.

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 463645 comment #13.

Comment 1

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

Engaging the community on this one: http://lists.w3.org/Archives/Public/wai-xtech/2009Mar/0058.html

Comment 2

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

I think Marco is right, for something like <table role="log"> we shouldn't override the html role (but still do the aria live stuff).

Comment 3

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

A little while back I checked with WAI-PF and there is no resistance with us using the native role for something like <table role="log">. I even got a few supportive replies.

A possible heuristic for nsAccessible::GetRole falling through to GetRoleInternal (native markup), is checking for the presence of aria-live.

I wonder if we should fall through if ROLE_NOTHING and mRoleMapEntry->liveAttRule

Comment 4

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

Attachment: WIP for discussion. All tests pass including a test_table_4 fix.

Comment 5

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

Sorry comment #3 wasn't quite finished, but the patch finishes my point :)

Comment 6

[alexander :surkov (:asurkov)]

I would say let's introduce field for landmark in nsRoleMapEntry

Comment 7

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

Attachment: patch

Alex, is this what you are thinking? It is a bit more bloat, but clearer and less fragile I think.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 377736 [details] [diff] [review]
patch

Yes, that what I was thinking of.

> PRUint32 nsARIAMap::gWAIRoleMapLength = NS_ARRAY_LENGTH(nsARIAMap::gWAIRoleMap);
> 
> nsRoleMapEntry nsARIAMap::gLandmarkRoleMap = {
>   "",
>   nsIAccessibleRole::ROLE_NOTHING,
>+  eUseMapRole,

landmarks do not override native role iirc.

> nsRoleMapEntry nsARIAMap::gEmptyRoleMap = {
>   "",
>   nsIAccessibleRole::ROLE_NOTHING,
>+  eUseMapRole,
>   eNoValue,

please confirm it's true for empty role (jast a bit lazy to dig through the code :) ).

>-  return mDOMNode ?
>-    GetRoleInternal(aRole) :
>-    NS_ERROR_FAILURE;  // Node already shut down
>+  return GetRoleInternal(aRole);

GetRoleInternal does't check on defunct and GetRole() as well.

> 
>-    testRole(accTable4, ROLE_NOTHING); // XXX: it's a bug, should be ROLE_TABLE
>+    testRole(accTable4, ROLE_TABLE);

would be nice to have more tests, in separate test I think.


Btw, new enum is boolean actually so it will be possible clearer if you use boolean overrideNativeRole flag instead.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 377736 [details] [diff] [review]
patch

This fixes the ChatZilla problem on 3.6a1pre nightly builds. Looks correct! r=me

Comment 10

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

(In reply to comment #8)
> (From update of attachment 377736 [details] [diff] [review])
> GetRoleInternal does't check on defunct and GetRole() as well.

Actually I added an IsDefunct check in GetRole in this patch ;)

I'll try to address the other comments in the next patch.

(In reply to comment #9)
> (From update of attachment 377736 [details] [diff] [review])
> This fixes the ChatZilla problem on 3.6a1pre nightly builds. Looks correct!
> r=me

Thanks for confirming the fix Marco :)

Comment 11

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

(In reply to comment #8)
> landmarks do not override native role iirc.

Right. Fixed in my tree thanks.

> please confirm it's true for empty role (jast a bit lazy to dig through the
> code :) ).

Yeah I mixed empty and landmark up. I've switched them. Thanks!

> would be nice to have more tests, in separate test I think.

I'll add some.

Comment 12

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

Spun off bug 493795 for a comprehensive treatment of landmark testing.

Comment 13

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

Attachment: patch2

You guys are awesome.

OK so the only ROLE_NOTHING here that doesn't pick up the native role is "presentation"; which makes sense right?

Surkov, I made the role rule a boolean in case it saves us some bytes.

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 379293 [details] [diff] [review]
patch2

r=me

Comment 15

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

Attachment: patch 2, with tests

I went ahead and added tests (bug 493795).

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 379580 [details] [diff] [review]
patch 2, with tests

r=me, cool thanks!

Comment 17

[alexander :surkov (:asurkov)]

log -> kUseNativeRole - sounds correct, used on HTML tables. Btw, why do we need to have role="log" on table?

marquee, timer -> kUseNativeRole - you are trying to keep lesser changes, it's ok but how much is this correct?

gEmptyRoleMap -> kUseNativeRole - wrong, it should use kUseMapRole

+ // Role rule to be used for weak roles (i.e. ROLE_NOTHING)

What does it mean, does ARIA log role is also weak role? Btw, roleRule flag is defined for all roles.

> *aRole = nsIAccessibleRole::ROLE_NOTHING;

initialize it before IsDefunct() check.

Would be nice to rethink comment for 

if (mRoleMapEntry->roleRule == kUseMapRole) {

+ // Note if we do expose ROLE_NOTHING, like with landmarks, this

we do not expose nothing role for landmarks, it's more clean to say what we do and why we do instead  of what we would do.

+ test_aria_vs_native_role.html \

probably test_aria_roles to be more consistent with mochitests names?

+ // "live" roles

would be nice to have something more descriptive. I get you now but I won't get this comment in half of year.

I like to have tests for empty role map

otherwise is good.

Comment 18

[Marco Zehe (:MarcoZ)]

(In reply to comment #17)
> log -> kUseNativeRole - sounds correct, used on HTML tables. Btw, why do we
> need to have role="log" on table?

a) role="log" implies aria-live="polite". In other words, someone using the role of "log" does not need to specify aria-live="polite", they get it by default.
b) ChatZilla uses that. But I believe role="log" could be used on any element, ChatZilla was just the "example in the wild" if you will.

Comment 19

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

(In reply to comment #18)
> (In reply to comment #17)
> > log -> kUseNativeRole - sounds correct, used on HTML tables. Btw, why do we
> > need to have role="log" on table?
> 
> a) role="log" implies aria-live="polite". In other words, someone using the
> role of "log" does not need to specify aria-live="polite", they get it by
> default.
> b) ChatZilla uses that. But I believe role="log" could be used on any element,
> ChatZilla was just the "example in the wild" if you will.

Right, I wanted our mochitest to cover the chatzilla case.

(In reply to comment #17)
Thanks Alexander, I'll work up a another patch later.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #19)

> Right, I wanted our mochitest to cover the chatzilla case.

partially you cover it  :  testRole("log_table", ROLE_TABLE); Do you want to test aria-live also?


(In reply to comment #19)

> (In reply to comment #17)
> Thanks Alexander, I'll work up a another patch later.

you're welcome. thank you.

Comment 21

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

Attachment: patch 3

(In reply to comment #17)
> marquee, timer -> kUseNativeRole - you are trying to keep lesser changes, it's
> ok but how much is this correct?

I think it is correct.

> gEmptyRoleMap -> kUseNativeRole - wrong, it should use kUseMapRole
> 
> + // Role rule to be used for weak roles (i.e. ROLE_NOTHING)
> 
> What does it mean, does ARIA log role is also weak role? Btw, roleRule flag is
> defined for all roles.

Comment updated. (re: log, yes weak :) see step 3 of http://www.w3.org/WAI/PF/aria-implementation/#mapping_role)

> > *aRole = nsIAccessibleRole::ROLE_NOTHING;
> 
> initialize it before IsDefunct() check.

Right, done.

> Would be nice to rethink comment for 
> 
> if (mRoleMapEntry->roleRule == kUseMapRole) {
> 
> + // Note if we do expose ROLE_NOTHING, like with landmarks, this
> 
> we do not expose nothing role for landmarks, it's more clean to say what we do
> and why we do instead  of what we would do.

This section had a really confusing comment to start with, I've simplified it in this patch, thanks.

> 
> + test_aria_vs_native_role.html \
> 
> probably test_aria_roles to be more consistent with mochitests names?

Sure, done.
> 
> + // "live" roles
> 
> would be nice to have something more descriptive. I get you now but I won't get
> this comment in half of year.

Updated :)

> 
> I like to have tests for empty role map

How? I see one strange case in nsAccessibilityService where we use gEmptyRoleMap... what case is this? (Follow up table bug?)

(In reply to comment #20)
> (In reply to comment #19)
> 
> > Right, I wanted our mochitest to cover the chatzilla case.
> 
> partially you cover it  :  testRole("log_table", ROLE_TABLE); Do you want to
> test aria-live also?

Not here :)  Note the live attribute for log is already checked in test_objectattrs.html

Thanks.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)

> > I like to have tests for empty role map
> 
> How? I see one strange case in nsAccessibilityService where we use
> gEmptyRoleMap... what case is this? (Follow up table bug?)

new bug is not necessary I think, it should be easy like

<table role="label">
  <tr>
    <td id="cell">cell</td>
  </tr>
</table>

testRole("cell", ROLE_NOTHING);

if I read code right way.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 379917 [details] [diff] [review]
patch 3

nice, r=me, just please add test for gEmptyRoleMap.

Comment 24

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

Added locally and confirmed it tests gEmptyRoleMap. Thanks!

Comment 25

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

Pushed! http://hg.mozilla.org/mozilla-central/rev/f2f7d5625759

Comment 26

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre (.NET CLR 3.5.30729)