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.

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

Comment on attachment 635152 [details] [diff] [review] Implement subroles for WAI-ARIA Landmarks. r= This is missing the "search" landmark. But otherwise OK.

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

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

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

(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 on attachment 635861 [details] [diff] [review] Implement subroles for WAI-ARIA Landmarks. f=marcoz Nice!

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

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

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

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