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

VERIFIED FIXED in mozilla16

Status

()

Core
Disability Access APIs
P2
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: hub)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86_64
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

6 years ago
Commonly used feature, but pages are useable without this initially also.
Priority: -- → P2
(Assignee)

Comment 2

5 years ago
Created attachment 635152 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=
(Assignee)

Updated

5 years ago
Attachment #635152 - Flags: feedback?(marco.zehe)
(Reporter)

Comment 3

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

This is missing the "search" landmark. But otherwise OK.
Attachment #635152 - Flags: feedback?(marco.zehe) → feedback+
(Reporter)

Comment 4

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

Comment 5

5 years ago
Created attachment 635459 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. r=
(Assignee)

Updated

5 years ago
Attachment #635152 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #635459 - Flags: review?(dbolter)
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.
Attachment #635459 - Flags: review?(dbolter)
(Assignee)

Comment 7

5 years ago
Created attachment 635861 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz
(Assignee)

Updated

5 years ago
Attachment #635459 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #635861 - Flags: review?(dbolter)
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)
Attachment #635861 - Flags: review?(dbolter) → review+
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.
(Assignee)

Comment 11

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

Comment 12

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

Nice!
(Assignee)

Comment 13

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

Comment 14

5 years ago
Created attachment 639173 [details] [diff] [review]
Implement subroles for WAI-ARIA Landmarks. f=marcoz
(Assignee)

Updated

5 years ago
Attachment #635861 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
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.
Attachment #639173 - Flags: review?(dbolter)
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)
Attachment #639173 - Flags: review?(dbolter) → review+
(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.
(Assignee)

Comment 18

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

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/163d51da4349
(Assignee)

Updated

5 years ago
Assignee: nobody → hub
https://hg.mozilla.org/mozilla-central/rev/163d51da4349
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Reporter)

Comment 21

5 years ago
Verified in Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0 2012-07-09
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.