Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ngAria : role="button" added with ng-click directive cannot be disabled and is dangerous #11580

Closed
pgayvallet opened this issue Apr 14, 2015 · 9 comments

Comments

@pgayvallet
Copy link

Automatically adding unwanted role="button" on elements with ng-click directives can be very dangerous with some screen reader's behaviour. This ngAria feature cannot be disabled in ngAria config, which force developper to add a role attribute to element with ng-click when they dont want ngAria to add the button role to their clickable elements.

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2015

Can you explain why this is dangerous?

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2015

/cc @marcysutton

@marcysutton
Copy link
Contributor

Some specific examples would help. There is a node blacklist preventing it from being added to certain elements, which can be expanded. I'd argue it's more dangerous to put ng-click on elements that aren't interactive, and it isn't difficult to override the role.

@pgayvallet
Copy link
Author

Sure, I can explain some specific example

We had the issue with a a list of radio button :
Each radio is composed of a div container, with an ng-click handler containing the radio button itself. (The ng-click handler was here for mobile device purpose, to increase the click zone to select the radio buttons - ng-click='set-model-to-value-of-radio, basically).

With this configuration ( div role=button label input type=radio /div) VoiceOver (and maybe NVDA too, but not 100% sure) simply dont understand there is nested elements, stoping at the button role, with make the radio list description broken on the screen reader.

Sure, adding role="presentation" to the div do fix the issue. But honestly, I could find 100 more case where I can use a click handler on an element with simply doesnt match the "button" role, and forcing the developer to verbosely add a dummy role simply to say "hey, plz, do not mess around with inappropriate button role" seems unecessary.

Allowing the feature to be disabled ( it's the ONLY one ngAria doesnt allow to disable ) seems... legitimate imho.

@marcysutton
Copy link
Contributor

Cool, thanks for the information. We can totally make that configurable.

@marcysutton marcysutton self-assigned this Apr 21, 2015
marcysutton added a commit to marcysutton/angular.js that referenced this issue Apr 22, 2015
marcysutton added a commit to marcysutton/angular.js that referenced this issue Apr 22, 2015
marcysutton added a commit to marcysutton/angular.js that referenced this issue Jun 30, 2015
marcysutton added a commit to marcysutton/angular.js that referenced this issue Jun 30, 2015
@OlsonDev
Copy link

I just ran into this myself. Until those commits are available on bower, I threw a role="presentation" tabindex="-1" on my container. Is there a declarative way to change the focus to my <md-switch> inside the container like a native <label for="some-id"> would do? Or do I have to write some JavaScript to do that in my ng-click handler?

In case my question isn't clear, this is a snippet I'm messing around with:

<div layout ng-click="fullscreen = !fullscreen;" role="presentation" tabindex="-1">
    <div flex layout layout-align="start center"><md-icon class="md-primary">fullscreen</md-icon> Fullscreen</div>
    <md-switch flex="5" class="md-primary" ng-model="fullscreen" aria-label="Fullscreen"></md-switch>
</div>

@marcysutton
Copy link
Contributor

@OlsonDev that's an acceptable approach for the wrapping element with role=presentation and tabindex. 👍

Your question is unrelated to the reported issue, so let's try to keep it short. Are you saying a click on the wrapping div should send focus to the switch? Given md-switch is a custom component, it lacks some of the native behavior of a label and input. You could watch fullscreen and if it changes value, send focus to the switch, or send focus directly to the switch when the div is clicked. The former option seems cleaner (especially if you need the fullscreen property for other things), but it relies on a watch.

@OlsonDev
Copy link

@marcysutton My question was unrelated to the reported issue, sure. But the OP's real-world example was near-identical to mine so I figured it was related enough to ask. Regardless, apologies if I hijacked the issue; I'll try to keep it short.

Correct, the wrapping element should send focus to the switch on click/tap. Fair that it's custom and doesn't support native events. I don't think watching fullscreen and focusing on change makes sense; if an external event causes it to change, it effectively focus-steals. However, delegating the interaction (down the DOM) in an event handler does make sense to me. To make this functionality reusable, I made a directive; I think it's pretty clean (albeit perhaps not feature complete). Check it out. @wayofspark, you may find it useful, too.

@marcysutton
Copy link
Contributor

That totally works. It's much easier to reason about in live demo form. :)
In your scenario, event delegation from the wrapping element enhances the experience for mouse users, while the switch itself is keyboard accessible and labeled. Nice one!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants