Last Comment Bug 718700 - [Mac] WAI-ARIA landmarks are not communicated to VoiceOver.
: [Mac] WAI-ARIA landmarks are not communicated to VoiceOver.
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Mac OS X
: P2 normal (vote)
: mozilla16
Assigned To: Hubert Figuiere [:hub]
:
: alexander :surkov
Mentors:
http://www.marcozehe.de
Depends on:
Blocks: osxa11y
  Show dependency treegraph
 
Reported: 2012-01-17 09:13 PST by Marco Zehe (:MarcoZ)
Modified: 2012-07-09 07:56 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement subroles for WAI-ARIA Landmarks. r= (1.53 KB, patch)
2012-06-20 17:56 PDT, Hubert Figuiere [:hub]
mzehe: feedback+
Details | Diff | Splinter Review
Implement subroles for WAI-ARIA Landmarks. r= (4.57 KB, patch)
2012-06-21 14:10 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Splinter Review
Implement subroles for WAI-ARIA Landmarks. f=marcoz (5.16 KB, patch)
2012-06-22 12:25 PDT, Hubert Figuiere [:hub]
dbolter: review+
Details | Diff | Splinter Review
Implement subroles for WAI-ARIA Landmarks. f=marcoz (5.14 KB, patch)
2012-07-04 14:00 PDT, Hubert Figuiere [:hub]
dbolter: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2012-01-17 09:13:44 PST
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) 2012-01-18 06:58:25 PST
Commonly used feature, but pages are useable without this initially also.
Comment 2 Hubert Figuiere [:hub] 2012-06-20 17:56:26 PDT
Created attachment 635152 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=
Comment 3 Marco Zehe (:MarcoZ) 2012-06-21 03:57:37 PDT
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) 2012-06-21 03:58:37 PDT
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] 2012-06-21 14:10:41 PDT
Created attachment 635459 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=
Comment 6 David Bolter [:davidb] 2012-06-22 08:13:31 PDT
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] 2012-06-22 12:25:15 PDT
Created attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz
Comment 8 David Bolter [:davidb] 2012-06-22 12:36:48 PDT
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) 2012-06-22 13:49:32 PDT
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) 2012-06-22 13:51:25 PDT
(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] 2012-06-22 14:44:39 PDT
(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) 2012-06-25 00:28:57 PDT
Comment on attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz

Nice!
Comment 13 Hubert Figuiere [:hub] 2012-06-25 11:46:42 PDT
Marco, if you want to play with it, there are builds at 

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/hfiguiere@mozilla.com-8779f3162001/
Comment 14 Hubert Figuiere [:hub] 2012-07-04 14:00:40 PDT
Created attachment 639173 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz
Comment 15 Hubert Figuiere [:hub] 2012-07-04 14:02:21 PDT
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] 2012-07-05 06:24:21 PDT
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] 2012-07-05 06:25:02 PDT
(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] 2012-07-05 13:15:12 PDT
(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 21 Marco Zehe (:MarcoZ) 2012-07-09 07:56:20 PDT
Verified in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0 2012-07-09

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