494807 - Do not expose a11y info specific to hyperlinks when role is overridden using ARIA

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090524 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090524 Minefield/3.6a1pre

When a hyperlink (HTML A tag) has its role overridden using ARIA (e.g. role="menuitem"), Gecko still exposes information specific to hyperlinks via accessibility APIs. For example, the accessible value is the HREF attribute and the linked state is set. As I understand it, the ARIA role should completely overide the role of the object. As it is no longer a link, it should not include this information.

Reproducible: Always

Steps to Reproduce:
1. Open the example URL provided.
2. Navigate to one of the menu items.
3. Check the accessible value and states.
Actual Results:  
The accessible value is a URL (the HREF of the hyperlink). The linked state is set.

Expected Results:  
The value should be empty and the linked state should not be set.

See this NVDA ticket for the discussion which motivated this request:
http://www.nvda-project.org/ticket/273#comment:4

Comment 1

[alexander :surkov (:asurkov)]

What does ARIA spec say about this? It might be reasonable since iirc we don't create table accessibles for HTML table if ARIA role is used.

Comment 2

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

I think this bug is valid. I think we create table accessibles for weak roles only (and log).

Comment 3

[alexander :surkov (:asurkov)]

We should create proposal for this, kind of map when accessibles from native markup are consumed by ARIA roles. For example,

<a role="menuitem"> SHOUDN'T be result of accessible for HTML a

However

<table role="grid"> SHOULD be result of accessible for HTML table.

Also, should we consume text accessible in the case of

<p role="grid">

and should we consume any unexpected children? Otherwise how can we deal with

<p role="grid"><div role="section">This is paragraph</div></p>?

Comment 4

[alexander :surkov (:asurkov)]

I started proposal at https://wiki.mozilla.org/Accessibility/ARIAConflictsWithNativeMarkup. You're feedback is appreciated.

Comment 5

[Marco Zehe (:MarcoZ)]

I did some grammatical corrections and wording clarifications without changing the meaning of the proposal. One thing that wasn't mentioned, but which should be added, is that "presentation" is definitely a strong ARIA role. Any html:table element with role "presentation" should not have a table interface created, for example.

Comment 6

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

Attachment: patch (minimal specific fix)

Alexander, this bug as described is making life hard for Yahoo and NVDA. I propose this fix to stop the bleeding, and for the more general approach to be done on a new bug as cases crop up.

Related discussion is happening between HTML5 and ARIA teams but may take a while.

Comment 7

[alexander :surkov (:asurkov)]

Generally our approach is to not create specific accessible in this case. If it's really urgent then I won't stop your patch. However I'll request to add XXX section to point this is temporary solution and to file new bug for this.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 406234 [details] [diff] [review]
patch (minimal specific fix)

my conditional r+

>+PRBool
>+nsHTMLLinkAccessible::IsLink()
>+{
>+  PRUint32 role = nsIAccessibleRole::ROLE_NOTHING;
>+  GetRole(&role);

nsAccUtils::Role

>+  return role == nsIAccessibleRole::ROLE_LINK;
>+}
>\ No newline at end of file

new line please

Comment 9

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

Attachment: patch - don't create link accessible for html:a@role

Alexander, I like your suggestion better. Thoughts?

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 406442 [details] [diff] [review]
patch - don't create link accessible for html:a@role

Ok, this approach is sufficient because this is unique place where we create nsHTMLLinkAccessible.

>   else if (tag == nsAccessibilityAtoms::a) {
>+    // links with aria roles can confuse screen readers (bug 494807)

nit: I don't think it's necessary to mention bug number. Otherwise it sounds like there is a bug we should fix.

>+    if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role))

Not correct. You should follow our draft proposal. Please add mochitests to test html:a having different roles.

cancelling review to see new patch and chosen approach to fullfil this.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 406442 [details] [diff] [review])
> Ok, this approach is sufficient because this is unique place where we create
> nsHTMLLinkAccessible.
> 
> >   else if (tag == nsAccessibilityAtoms::a) {
> >+    // links with aria roles can confuse screen readers (bug 494807)
> 
> nit: I don't think it's necessary to mention bug number. Otherwise it sounds
> like there is a bug we should fix.

Yeah the devs I've talked to about putting bug numbers in source code are divided pretty evenly. Note I could say closed bug...

> 
> >+    if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role))
> 
> Not correct. You should follow our draft proposal. Please add mochitests to
> test html:a having different roles.
> 

I looked at the proposal. Are you suggesting we should check the value of role? Are you worried that we should let <a role="link"> create an nsHTMLLinkAccessible? I just don't know why that case would ever come up but I can handle it (its more code for a bogus case really).

Or are you concerned about the following line where I create a generic html accessible instead of a more generic accessible?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> Yeah the devs I've talked to about putting bug numbers in source code are
> divided pretty evenly. Note I could say closed bug...

:) nice, I assumed this. Ok, if you prefer this and will use "closed" word then it's ok.

> > 
> > >+    if (content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::role))
> > 
> > Not correct. You should follow our draft proposal. Please add mochitests to
> > test html:a having different roles.
> > 
> 
> I looked at the proposal. Are you suggesting we should check the value of role?
> Are you worried that we should let <a role="link"> create an
> nsHTMLLinkAccessible? I just don't know why that case would ever come up but I
> can handle it (its more code for a bogus case really).

as we discussed on irc, you should use rolemap and check it's role, if it's nothing or link then create link accessible, otherwise hypertext.

> Or are you concerned about the following line where I create a generic html
> accessible instead of a more generic accessible?

No, hyper text accessible is fine.

Comment 13

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

Attachment: patch3 (more general)

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 406703 [details] [diff] [review]
patch3 (more general)


>+    // Only some roles truly enjoy life as nsHTMLLinkAccessibles, for details
>+    // see closed bug 494807

nit: dot or should be combined with

>+    // use markup

?

> 		test_value.xul \
>+		test_value.html \

nit: please put it before XUL test

otherwise looks ok, thank you.

Comment 15

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

landed 1.9.3 http://hg.mozilla.org/mozilla-central/rev/d490e107ea2a

Comment 16

[Marco Zehe (:MarcoZ)]

Jamie, can you verify that this makes life easier for you and Victor?
Do we want this for 3.6? If it really makes such a big difference, I can imagine we would. If so, David, what's the risk assessment?

Comment 17

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

(In reply to comment #16)
> Jamie, can you verify that this makes life easier for you and Victor?
> Do we want this for 3.6? If it really makes such a big difference, I can
> imagine we would. If so, David, what's the risk assessment?

Very low risk.

Comment 18

[Todd Kloots]

Hey Marco - 

This would definitely make my life easier, and by extension, Victor's.  :)   I've been hacking around this bug for a while by using "removeAttribute" to remove the "href" of attribute anchor elements after I apply a role.  That hack, however, results in anchors not firing click events when the user presses the enter key.  So, I have to hack around that as well.  If I could pull all these out, that would be awesome.  

Getting this bug fixed would also help me as an ARIA evangelist.  Many times when I am giving talks on using ARIA with Progressive Enhancement I end up walking developers through the aforementioned hack as a practical piece of advice.  While I think such practical advice is necessary, it'd be nice if I didn't have to teach such hacks.  The more perceivable hurdles we can remove for developers and make ARIA "just work" is a win.  

- Todd

Comment 19

[Marco Zehe (:MarcoZ)]

Comment on attachment 406703 [details] [diff] [review]
patch3 (more general)

Requesting approval in light of Todd's explanation in comment#18 and David's risk assessment in comment#17.

Comment 20

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

Yahoo! wants this.

Comment 21

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

Marco we probably need to request 1.9.2 blocking on the bug itself to make it in -- it will be a hard sell at this point.

Comment 22

[Marco Zehe (:MarcoZ)]

Requesting blocking based on the strong Yahoo! interest and also David's comment #21.

Comment 23

[Marco Zehe (:MarcoZ)]

Pushed to 1.9.2 on David's behalf:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/807ad771a809

Comment 24

[Marco Zehe (:MarcoZ)]

Verified in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3 (.NET CLR 3.5.30729)