angular / material

Material design for AngularJS
https://material.angularjs.org/
MIT License
16.55k stars 3.39k forks source link

Allow developers to disable attributes injected by ngAria #600

Closed marcysutton closed 9 years ago

marcysutton commented 9 years ago

One nice feature of ngAria is that developers can disable specific attributes that conflict with their own implementation, for example, if they already manage tabindex. Because of our mix of ngAria and custom accessibility support, opting out of attributes managed both by us and by ngAria is not possible. For example, see these two code snippets:

var app = angular.module( 'app', ['ngAnimate','ngMaterial'], 
  function config($ariaProvider) {
      $ariaProvider.config({
        tabindex: false,
        ariaInvalid: false
   });
});
<md-radio-group ng-model="data.group1">
    <md-radio-button value="Apple" aria-label="Apple">Apple</md-radio-button>
    <md-radio-button value="Banana" aria-label="Banana">Banana</md-radio-button>
    <md-radio-button value="Mango" aria-label="Mango">Mango</md-radio-button>
</md-radio-group>

tabindex is added to <md-radio-group> by Angular Material so only aria-invalid is suppressed, not tabindex. ngAria leaves the existing tabindex intact, as it should.

As much as it seems like a bad idea to allow people to disable accessibility support for attributes, should we create a mechanism to support custom disabling of attributes in this way, since it is supported by ngAria?

Example on Plunkr.

Related: https://github.com/angular/material/issues/583 (bug)

ThomasBurleson commented 9 years ago

Can we not check the configuration of ngAria? Developers can use

$AriaProvider.config({ 'tabindex':false });

Then our $mdAria.expect( ) can get a reference to the configuration settings and internally check if that option is enabled.

var shouldCheckAriaAttribute =  $AriaProvider.$get().config;
paynecodes commented 9 years ago

@marcysutton good find. I'm experiencing this issue now. :+1: in hopes that it doesn't get pushed back further than 0.8.0.

marcysutton commented 9 years ago

@ThomasBurleson we don't actually expose the $ariaProvider in ngMaterial, we only pull in ngAria as a dependency (https://github.com/angular/material/blob/master/scripts/gulp-utils.js#L151). It's an unusual setup, so I can't quite figure out how to expose it. I'd love your input.

https://docs.angularjs.org/api/ngAria/provider/$ariaProvider

ThomasBurleson commented 9 years ago

@marcysutton - I have some ideas. Let's talk after ng-conf.

paynecodes commented 9 years ago

@ThomasBurleson @marcysutton Did you all have a chance to talk about this? :)

marcysutton commented 9 years ago

@jpdesigndev can you elaborate on what issues you ran into? We build our components to be standalone-accessible, occasionally leveraging ngAria. I'm actually not so sure now that you should be able to override that.

marcysutton commented 9 years ago

I don't think this is a good idea anymore. If it comes up again and we can make a good case for it, we'll reopen.

paynecodes commented 9 years ago

@marcysutton Sorry, I missed the previous comment notification. So, I'm using ui-select which works fine without ngAria. With ngAria, tabindex is automatically added to a nested DOM node where it shouldn't therefore going into or out of the control requires an extra tab. I'm going off the top of my head here. It's been a while since I applied a workaround.

Why do you think this is no longer a good idea? Having a bit of control to override sensible defaults seems like a good idea to me.

marcysutton commented 9 years ago

@jpdesigndev a demo would help, so I can dig in and look at the actual issue. What we don't want to enable is inaccessibility of the Angular Material components, which leverage ngAria–those instances of tabindex need to stay.

Tabindex dynamically added outside of components we control (like in a 3rd-party component) sounds like a byproduct of ngAria consumed in Angular Material. However, we do not support 3rd-party components. I'd recommend switching to the Angular Material select.

benshope commented 9 years ago

Is it possible to remove ngAria from ngMaterial completely?

The extra bower requirement is a slightly annoying - and now ngAria appears to be printing warnings to the console - which is quite annoying.

Sorry if there is an easy "off" switch that I am missing.

smashercosmo commented 9 years ago

Yep, +1 for possibility to use angular.material without ngAria. In our app we don't need any accessibility stuff at all.

marcysutton commented 9 years ago

No, ngAria is a required dependency. But it isn't printing warnings–Angular Material is. If you are seeing warnings in the console, you need to add aria-label attributes to your components. This is non-negotiable. It should at least point you to the specific nodes in question to help you locate the problem. If there are bugs with the ngMaterial logging utility, though, we want to know.

patrickhlauke commented 9 years ago

In our app we don't need any accessibility stuff at all.

"Our restaurant is up a steep flight of stairs...so we don't need a wheelchair-accessible toilet"

benshope commented 9 years ago

I totally understand and respect that ngAria is included and active by default. But it would be really nice if there were a less-publicized fork of ngMaterial without ngAria, released by those who maintain ngMaterial. At the very least there should be a way to switch off all warnings in the app.config() (although... I'm not sure - is there?)

For example, my app does nothing but show visuals of data using d3.js. It is inherently inaccessible to people with impaired vision.

@patrickhlauke - If I were the owner of a restaurant - I would hope to be encouraged to choose accessibility, but allowed to make a pragmatic decision. Software developers are a nice bunch; everyone wants to do the right thing.

marcysutton commented 9 years ago

@benshope if we added an option to disable warnings, it would effectively enable developers to create inaccessible web applications. It's just not something we are going to do. What we can do, however, is look at specific use cases where the warnings don't make sense. Can you provide a Codepen or Plunker that shows your use case?

There has to be a way to enhance your data visualization for visually impaired users, rather than completely throwing them under the bus. This is actually an opportunity to figure out what the best practices could be.

benshope commented 9 years ago

@marcysutton - I am looking to remove tags like aria-label="Home" from my main menu.

Adding screen-reader accessibility to this app (even to the main menu) would be pointless because the data is already in it's most accessible format, as tables of data, on another website.

Perhaps the ideal behavior in my situation would be to have <body aria-redirect="Please visit this other website"> and then any content within aria-redirect would not throw an accessibility-related error.
The aria-redirect solution has several benefits:

I realize it is unlikely the idea of an aria-redirect will gain traction in this forum, but I will put it forward anyways.

marcysutton commented 9 years ago

Shipping users off to another website is your decision, but it's not something we're going to recommend as a framework. Just add the labels and then you won't get the warnings...You can provide a text link on the page for people who want the other version. I'm still not convinced why you would advocate removing accessibility for an entire app when only part of it is inaccessible-a better approach would be to tell people about the alternative content somewhere, even a visually hidden link.

MylesBorins commented 9 years ago

Please let me know if I am missing something. Is the problem that you don't want to do a little extra work to avoid errors... and the extra bower dependency is giving you the jibbilies?

@benshope I hope you realize how entitled you are coming off here.

If you want a version without aria, which might i add goes completely against the fundamental ideology of the project, there is absolutely nothing stopping you from forking the project and making the changes yourself. Why you think the project maintainers would do this for you is beyond me.

The downside to this... all the time you will have to spend maintaining these changes and keeping things up to date. I wonder if just avoiding the errors to begin with would take less time.

benshope commented 9 years ago

@marcysutton Perhaps a better name would be aria-inaccessible="My excuse for the situation". I am not advocating that the framework encourage magical redirects. Just that it allow totally inaccessible apps and parts-of-apps.

@thealphanerd Sorry, if I am coming across as insensitive. But yes, I feel strongly that the framework should take a stance that encourages, but does not require, insertion of unsemantic boilerplate code into an otherwise-beautiful and concise ui framework. I would fork the project, but what you said is absolutely correct - maintaining it would be a PITA.

A framework should be absolute in some ways, opinionated in some ways, and agnostic in some ways. Aria has been placed in the wrong basket.

Also, not meaning to argue, just to voice an opinion. I know sometimes I come across a bit strong :smile:

MylesBorins commented 9 years ago

The argument that aria should be a nice to have is inherently flawed and files you in the noise category as far as I'm concerned

patrickhlauke commented 9 years ago

Perhaps a better name would be

before we try to bikeshed any more aria properties, might be worth pointing out that it's an actual standard, so not just something that can be made up... http://www.w3.org/TR/wai-aria/

unsemantic boilerplate code

the irony here is that adding aria is actually infusing the unsemantic tagsoup that's generally spat out by angular with some semblance of semantics that can then get conveyed to assistive technologies...

ryanflorence commented 9 years ago

If you are okay with doing something terrible like dismissing accessibility, then just console.warn = function() {} and go to bed at night feeling bad for at least something :P

dylanb commented 9 years ago

@benshope every single application can be made accessible, failure to want to do so is pure laziness. It is the sign of an inferior developer.. Do you also advocate for removing security features from frameworks? Perhaps we should remove the unicode support from JavaScript too. ASCII characters are so much more memory-friendly! Hey, I even have this Windows application that supports unicode, so keep those useless extra characters away from my web site!

ryanflorence commented 9 years ago

A framework should be absolute in some ways, opinionated in some ways, and agnostic in some ways. Aria has been placed in the wrong basket.

Material is a UI framework. UI does not mean "Visual Design" but "User Interface". The interface for some folks is how it sounds. So, making your UI not just look beautiful, but also sound beautiful is absolutely, without question, in the right basket.

Sorry, if I am coming across as insensitive

I wouldn't call it insensitive but ignorant to many users of the web. Use this thread and framework as an opportunity to level up on your craft!

At a previous job one of our front-end devs was completely blind. We'd get steaming piles of crap HR apps that he couldn't use. ngAria is doing you an enormous favor, you barely have to learn anything to not create another warm, steamy pile.

Level up! http://www.w3.org/TR/wai-aria-practices/

benshope commented 9 years ago

@ryanflorence - Thank you! console.warn = function() {} is an admittedly-shady way to not-see console errors for the time being. I will very likely use ngAria eventually - what you say makes sense :+1:

@dylanb I just like to write as few lines as humanly possible - and I don't think that should be a contentious philosophy. If my app does not need security features, and these features require me to write more code, then a good framework will allow me to disable them. Things like security, test coverage, and accessibility are all very nice, and very non-critical in certain situations. Writing less code is a gesture of respect towards other developers who will eventually read your code.

ctoth commented 9 years ago

@benshope It may be a gesture of respect to other developers reading your code (though I don't quite see how this could be), but it's a gesture of a totally different kind to the blind guy like me who comes across your inaccessible turd later. A gesture a lot more akin to an upraised middle finger.

marcysutton commented 9 years ago

I would like to remind everyone of our Code of Conduct, which specifically forbids unprofessional behavior in communication channels. https://github.com/angular/code-of-conduct/blob/master/CODE_OF_CONDUCT.md

That said, @benshope, I can't believe you would seriously consider removing console output instead of just adding accessibility labels. If I had the ability to lock this issue, I would, as it has devolved into something that is not productive. What you are asking us to support goes against a core philosophy of the Angular Material project–the framework is intended to support a large user base, not a discriminatory subset. We are not going to disable accessibility features just so that you can write less lines of code. End of story.

EladBezalel commented 9 years ago

This is absurd.. There are projects that don't need aria support.. You should supply a way to disable it and warn that this is disabled..

Adding additional attributes to large amount of elements is extra bytes that I can't allow to have in my project..

yatil commented 9 years ago

@EladBezalel: Are your projects used by humans? If they do, they need ARIA support.