718700 - [Mac] WAI-ARIA landmarks are not communicated to VoiceOver.

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

The WAI-ARIA landmarks "banner", "main", "search", "complementary" etc. are not communicated to VoiceOver. They can be found, for example, on my blog http://www.marcozehe.de.

Comment 1

[Marco Zehe (:MarcoZ)]

Commonly used feature, but pages are useable without this initially also.

Comment 2

[Hubert Figuiere [:hub]]

Attachment: Implement subroles for WAI-ARIA Landmarks. r=

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 635152 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=

This is missing the "search" landmark. But otherwise OK.

Comment 4

[Marco Zehe (:MarcoZ)]

Also, we internally map the HTML5 nav element to "navigation", too. From what I can see, this part is missing from this patch.

Comment 5

[Hubert Figuiere [:hub]]

Attachment: Implement subroles for WAI-ARIA Landmarks. r=

Comment 6

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

Comment on attachment 635459 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=

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

::: accessible/src/mac/mozAccessible.mm
@@ +443,5 @@
> +  if (rv == NS_OK)
> +    nsAccUtils::GetAccAttr(attributes, nsGkAtoms::xmlroles, xmlRoles);
> +
> +  if (!xmlRoles.IsEmpty()) {
> +    if (xmlRoles.EqualsLiteral("navigation"))

Although this should work for 98% of cases you need to handle the multiple (fallback) roles case (we should add test coverage for that). For example:
<div role="navigation group">foo</div>

(See first part of http://www.w3.org/TR/wai-aria-implementation/#mapping_role)

Note: I'd review a test for that (probably in test_xml-roles.html.

@@ +504,5 @@
> +      return utils::LocalizedString(NS_LITERAL_STRING("complementary"));
> +    if ([subrole isEqualToString:@"AXLandmarkContentInfo"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("content"));
> +    if ([subrole isEqualToString:@"AXLandmarkNavigation"])
> +      return utils::LocalizedString(NS_LITERAL_STRING("navigation"));

This looks right. Please keep the order of these the same in subrole, here, and in properties. Maybe alphabetic is easiest.

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Implement subroles for WAI-ARIA Landmarks. f=marcoz

Comment 8

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

Comment on attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

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

r=me with the comment addressed.

::: accessible/src/mac/mozAccessible.mm
@@ +431,5 @@
>  #undef ROLE
>  }
>  
>  - (NSString*)subrole
>  {

I'm wondering if your whole addition to subrole should be inside the mRole switch that follows for a case roles::GROUP. Does that make sense?  (worried about perf)

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

>+  // XXX maybe we should cache the subrole.

no. if you need this to b faster we should seperate getting the xml role out of GetAttributes() which might be a good idea anyway.

mGeckoAccessible->GetAttributes(getter_AddRefs(attributes));
>+  if (rv == NS_OK)
>+    nsAccUtils::GetAccAttr(attributes, nsGkAtoms::xmlroles, xmlRoles);

NS_SUCCEEDED()

>+
>+  if (!xmlRoles.IsEmpty()) {
>+    nsWhitespaceTokenizer tokenizer(xmlRoles);

this if doesn't really serve much of a purpose, initializing a tokenizer with a reasonable string meaning not "         f" or something is very cheap.

>+
>+    while (tokenizer.hasMoreTokens()) {
>+      nsAutoString token(tokenizer.nextToken());

const nsDependantSubstring

>+
>+      if (token.EqualsLiteral("banner"))
>+        return @"AXLandmarkBanner";
>+      if (token.EqualsLiteral("complementary"))
>+        return @"AXLandmarkComplementary";

nit, blank line between ifs.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #8)
> Comment on attachment 635861 [details] [diff] [review]
> Implement subroles for WAI-ARIA Landmarks. f=marcoz
> 
> Review of attachment 635861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comment addressed.
> 
> ::: accessible/src/mac/mozAccessible.mm
> @@ +431,5 @@
> >  #undef ROLE
> >  }
> >  
> >  - (NSString*)subrole
> >  {
> 
> I'm wondering if your whole addition to subrole should be inside the mRole
> switch that follows for a case roles::GROUP. Does that make sense?  (worried
> about perf)

its not immediately clear to me why that would be correct.  That said other than the fact GetAttributes() is a terribly inefficient way to get a single attribute I don't see anything to worry about here.

Comment 11

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > I'm wondering if your whole addition to subrole should be inside the mRole
> > switch that follows for a case roles::GROUP. Does that make sense?  (worried
> > about perf)
> 
> its not immediately clear to me why that would be correct.  That said other
> than the fact GetAttributes() is a terribly inefficient way to get a single
> attribute I don't see anything to worry about here.

To be honest I'm not sure the value of mRole is guarranted to be roles::GROUP.
Yeah GetAttributes() is what worries me.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

Nice!

Comment 13

[Hubert Figuiere [:hub]]

Marco, if you want to play with it, there are builds at 

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8779f3162001/

Comment 14

[Hubert Figuiere [:hub]]

Attachment: Implement subroles for WAI-ARIA Landmarks. f=marcoz

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 639173 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

The only change with previous patch is that I null check attributes in [mozAccessible subrole] as we can have success and still get nsnull. It was causin a crash with another fix rolled in later.

Comment 16

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

Comment on attachment 639173 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

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

r=me. Please make the suggested optimization if you agree.

(You guys were right - I don't know what I was thinking about the group role thing)

::: accessible/src/mac/mozAccessible.mm
@@ +438,5 @@
> +
> +  // XXX maybe we should cache the subrole.
> +  nsAutoString xmlRoles;
> +  nsCOMPtr<nsIPersistentProperties> attributes;
> +

It just occurred to me an optimization you should probably make here is to check the gecko accessible for HasARIARole() before proceeding with the attribute checking.

@@ +441,5 @@
> +  nsCOMPtr<nsIPersistentProperties> attributes;
> +
> +  nsresult rv = mGeckoAccessible->GetAttributes(getter_AddRefs(attributes));
> +  if (NS_SUCCEEDED(rv) && attributes)
> +    nsAccUtils::GetAccAttr(attributes, nsGkAtoms::xmlroles, xmlRoles);

Please add a comment like:
// XXX we don't need all the attributes (see bug 771113)

Comment 17

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

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 635861 [details] [diff] [review]
> Implement subroles for WAI-ARIA Landmarks. f=marcoz
> 
> >+  // XXX maybe we should cache the subrole.
> 
> no. if you need this to b faster we should seperate getting the xml role out
> of GetAttributes() which might be a good idea anyway.

Agreed. Filed as bug 771113.

Comment 18

[Hubert Figuiere [:hub]]

(In reply to David Bolter [:davidb] from comment #16)

> ::: accessible/src/mac/mozAccessible.mm
> @@ +438,5 @@
> > +
> > +  // XXX maybe we should cache the subrole.
> > +  nsAutoString xmlRoles;
> > +  nsCOMPtr<nsIPersistentProperties> attributes;
> > +
> 
> It just occurred to me an optimization you should probably make here is to
> check the gecko accessible for HasARIARole() before proceeding with the
> attribute checking.

It seems to not work as HasARIARole() return whether we have a mRoleMapEntry, which in the case of the <nav> markup, we don't.

Comment 19

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/163d51da4349

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/163d51da4349

Comment 21

[Marco Zehe (:MarcoZ)]

Verified in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0 2012-07-09