Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ember.component support for aria-label, aria-labelledby, and aria-describedby #242

Closed
wants to merge 4 commits into from

Conversation

MelSumner
Copy link
Contributor

@MelSumner MelSumner commented Jul 31, 2017

Requesting specific consideration for the three ARIA attributes that I am putting in to every component I build. It would help on multiple levels to have this supported by default in Ember.component, like ariaRole.

(inspiration for this RFC also comes from this issue: emberjs/ember.js#13224 - but I did not see a resulting RFC)

Rendered

@mmun
Copy link
Member

mmun commented Aug 1, 2017

I like the idea here. I'm also thinking about a generic solution that let's you assign any attribute.

One idea is to use a subexpression as a final positional argument:

{{my-component (html-attributes aria-labelledby="label-name")}}

There's already some precedence with link-to and the query-params subexpression.

@buschtoens
Copy link
Contributor

buschtoens commented Aug 1, 2017

While making it easier to improve accessibility by default is a noble goal, I feel like this is the wrong approach. Just as I feel like ariaRole should not have been included in Ember.js itself.

Introducing default attribute bindings for every component incurs a (arguably small) performance hit and more importantly increases API surface area. Also, where do you draw the line of which attributes should be bound by default and which not?

As @mmun already suggested, I would rather improve the primitives for binding attributes to make it less of a hassle. Depending on the use case, there are two scenarios:

You either want a component to declare its HTML attributes itself (maybe in dependence of data attrs that were passed into the component) or you want to pass HTML attributes when invoking the component.

AFAIK Glimmer components will elegantly solve the latter by making a distinction between data attrs and HTML attributes. (Correct me, if I'm wrong here. I can't find the RFC / docs.)

<my-component aria-role="button" @user=alice>

And ember-decorators/ember-decorators#133 would give you a really nice idiomatic way of defining attribute bindings. No more attributeBindings arrays.

import Component from '@ember/component';
import { computed } from 'ember-decorators/object';
import { attribute } from 'ember-decorators/component';

export default class extends Component {
  @attribute('aria-label') label = 'foo';

  // or even as a computed property:
  @attribute('aria-label')
  @computed('foo', 'bar')
  label(foo, bar) {
    return `${foo}-${bar}`;
  }
}

In the meantime, while this is all being fleshed out, you could define default attribute bindings for your own components:

// app/components/accessible-component.js
import Component from '@ember/component';

export default Component.extend({
  attributBindings: [
    'ariaLabel:aria-label',
    'ariaLabelledBy:aria-labelledby',
    'ariaDescribedBy:aria-describedby'
  ]
});

// app/components/foo-component.js
import AccessibleComponent from 'my-app/components/accessible-component';

export default AccessibleComponent.extend({
  ariaLabel: 'something'
});

@miguelcobain
Copy link
Contributor

I'd just like to add to @buschtoens reply that using
a Mixin is also a good alternative. Reopening the component class might be the best "do and forget" way.

@buschtoens
Copy link
Contributor

@miguelcobain I was gonna suggest the same, but was unsure where to actually put the code that reopens the Component class. I'm happy you still added it, so I can ask. 😜

Where would you reopen the class? In the Mixin file itself, in an initializer or maybe in a file in vendor? IMHO the docs could go more into detail about reopening Ember framework classes.

@miguelcobain
Copy link
Contributor

@buschtoens I see most people (including myself) reopening in initializers. At least in addons, an initializer seems to be the only way to go.

@simonihmig
Copy link
Contributor

Regarding reopening I would tend to put it in a vendor file, and this.import (or app.import for older ember-cli). Reason: initializers don't run automatically in integration tests, so if your tests cover any functionality affected by the mixin, they would fail. Or you would have to call the initializer explicitly in some before hooks. Example of vendor solution: https://github.com/simplabs/ember-test-selectors/blob/master/vendor/ember-test-selectors/patch-component.js

@mmun
Copy link
Member

mmun commented Aug 1, 2017

@buschtoens

AFAIK Glimmer components will elegantly solve the latter by making a distinction between data attrs and HTML attributes. (Correct me, if I'm wrong here. I can't find the RFC / docs.)

This is correct. The docs are on the glimmerjs.com website, but expect to see an RFC here before we pull glimmer-style component invocation into Ember.

@MelSumner
Copy link
Contributor Author

@buschtoens

Two items to consider:

  • If you look in the "alternatives" section, you will see that I do currently add these using the attributeBindings method.

  • I am specifically suggesting we draw the line at the most important/common attribute that is important for assistive technology: a machine readable name.

Reading the other comments has me curious: what's the "bigger-yet-still-negligible" perf hit, having an unused property or reopening a component?

@Robdel12
Copy link

Robdel12 commented Aug 1, 2017

Just as I feel like ariaRole should not have been included in Ember.js itself

Can you explain your position on this?

Currently in ember each time I want to use aria attributes I have to set up the attribute bindings so ember components can render those aria attributes. Writing an initializer to extend ember is a hack imo. It's a hack to fill a gap in the framework and none of the greater community benefits from it.

I'm 100% on board with adding at least these aria attributes. I would expect to write the following and not have to do any extra work to make it work:

{{fake-button ariaLabel="Click me!" ariaRole="button"}}

What ember should render:

<div role="button" aria-label="Click me!"></div>

I want to understand your pushback. I do agree current ember components make it hard to add any attributes. This is where accessibility can drive a better API for everyone (not just for a11y). But I feel strongly that this is something Ember needs to do right, out of the box, in order to complete the "SDK of the web" dream.

To sum this up: single page apps have a ton of accessibility problems. Accessibility without a framework is hard enough, but it becomes even harder when you have to fill in the gaps of the framework to do it. It makes the barrier of entry even higher, which discourages devs from even trying.

@buschtoens
Copy link
Contributor

buschtoens commented Aug 1, 2017

@MelSumner

If you look in the "alternatives" section, you will see that I do currently add these using the attributeBindings method.

To me that reads like the "Alternatives: do nothing" approach I see often with RFCs. 😅

I was trying to address that with my remarks on inheriting from a common AccessibleComponent to provide a remedy that works right now. I think @miguelcobain and @simonihmig further improved on that by suggesting that users who want to bind aria-* attributes (or any attributes for that matter) should reopen the framework provided Component class.

From your POV, is there any disadvantage to baking this right into the framework?

I am specifically suggesting we draw the line at the most important/common attribute that is important for assistive technology: a machine readable name.

I have zero statistics or research to back this up, but from a purely anecdotal standpoint I would argue that the majority of Ember.js users cares way too little about a11y. Especially as a newcomer to web development or Ember.js chances are that you've never heard of aria-*.

They might argue that an attribute like title is way more common / useful and should be bound by default instead.

Reading the other comments has me curious: what's the "bigger-yet-still-negligible" perf hit, having an unused property or reopening a component?

The property ariaRole isn't really "unused". It's not implemented as a classic attribute binding, but baked right into curly components. The three additional bindings you propose would / should be added in the same way. So now we're at 4 potentially often unused bindings that are created for every component. I'm probably over-exaggerating here though. A benchmark would give certainty. 🤔

Ember.Object.reopen() delegates to Mixin.reopen(). Since Mixins are used all over the Ember framework, I would assume that they are fairly performant.

All in all, using default baked in bindings is probably a bit faster compared to attributeBindings, since you don't have to jump through these hoops when creating the element. Either way – having default bindings and not using them at all or manually setting these bindings up by reopening the Component class – the performance hit both ways is probably negligible to immeasurable.

So my main points of critique are:

  1. Ember's current sentiment is: "Keep it slim. Only provide the right primitives. Everything else is implemented by addons." IMHO this also applies here, since you already have all the required tools at hand: Component.reopen()

  2. Most people probably won't use this feature (zero statistics!). Hence I don't think increasing the API surface area is reasonable. Component is the central framework class I use most often every day. If possible I would rather slim it down in future versions.

@buschtoens
Copy link
Contributor

buschtoens commented Aug 1, 2017

@Robdel12

The problem you are referring to (passing plain HTML attributes is hard) has a broader scope and is not limited to aria-* attributes. That's why I feel like limiting the solution to this scope only is the wrong path.

We should rather improve the primitives, which Glimmer Components try to do.

{{fake-button aria-label="Click me!" role="button" @someDataAttr="foo"}}

Just as I feel like ariaRole should not have been included in Ember.js itself

Can you explain your position on this?

Currently in ember each time I want to use aria attributes I have to set up the attribute bindings so ember components can render those aria attributes. Writing an initializer to extend ember is a hack imo. It's a hack to fill a gap in the framework and none of the greater community benefits from it.

You can use the same argument for making title a default binding.

@fivetanley
Copy link
Member

fivetanley commented Aug 1, 2017

The problem you are referring to (passing plain HTML attributes is hard) has a broader scope and is not limited to aria-* attributes. That's why I feel like limiting the solution to this scope only is the wrong path.

Aria attributes are a web standard like any other, and most importantly Ember as an ecosystem is about good defaults for letting you use web standards to make great experiences for your users. Making accessibility hard doesn't seem like a good default.

While that problem in particular is hard, it's also being addressed with glimmer components. I don't think it's worth blocking this feature that people have needed and requested for as long as I've been doing Ember. When an accessibility problem comes up, a common answer is to adjust other attributes like tabindex, or to use an aria- attribute. Using ARIA isn't an edge case.

We should rather improve the primitives, which Glimmer Components try to do.

Changes in Glimmer take a long time to go through the RFC cycle and then be implemented in Glimmer/Ember. If the primitive is non-accessible to Ember developers now or in the immediate future, what should people do? Monkey-patch the framework in ways that it could break when glimmer components finally land? I think we'll see curly components exist for a long while after Glimmer components are in Ember core. This use case is solvable in both curly components and glimmer; I don't see why users should have to pick one.

You can use the same argument for making title a default binding.

I think that's more of a strawman. aria attributes are a lot more common than you'd think and more generically applicable. I think that's why this RFC is good! It limits us to just ARIA things.

@ssriera
Copy link

ssriera commented Aug 1, 2017

Most people probably won't use this feature (zero statistics!). Hence I don't think increasing the API surface area is reasonable. Component is the central framework class I use most often every day. If possible I would rather slim it down in future versions.

The reason people don't use aria-* most the time is because they are not aware of its existence nor of its uses. I agree its nice to keep the API slim but accessibility should be baked into the framework. We should be careful about comparing accessibility with things like whether or not we have title, it's not an optional feature. I think people would start being more mindful about accessibility and slowly implementing accessible components if the path has been laid out for them.

@fivetanley
Copy link
Member

I have zero statistics or research to back this up, but from a purely anecdotal standpoint I would argue that the majority of Ember.js users cares way too little about a11y. Especially as a newcomer to web development or Ember.js chances are that you've never heard of aria-*.

I think this is a problem with the web development community in general. If the goal is to engage developers on accessibility, locking them out of accessibility features doesn't seem like a way to empower developers to care more. Instead, they'll just be frustrated and not try to continue to try and learn. It takes a certain amount of googling to even find the answer to attribute bindings, let alone in the context of accessibility.

Ember's current sentiment is: "Keep it slim. Only provide the right primitives. Everything else is implemented by addons." IMHO this also applies here, since you already have all the required tools at hand: Component.reopen()

I don't think Ember has really ever embraced monkey patching the core primitives, even in the earlier days where we used to do it all the time 😛 . For the 3 attributes provided, this might not even be that big of a change to the default list of attribute bindings. I don't think we're doing anyone a huge favor by leaving them out.

@fivetanley
Copy link
Member

fivetanley commented Aug 1, 2017

I personally really like @mmun's suggestion to make it convenient to do attribute bindings in curly components using a helper here: #242 (comment).

Like with aria, I think it's a matter of legacy reasons why the initial set of default attribute bindings was limited/not generated at runtime.

@rwjblue
Copy link
Member

rwjblue commented Aug 1, 2017

Agreed. I quite like @mmun's proposal also. It elegantly solves both the specific issue this RFC is trying to address and the general issue of "there are lots of attributes that other component authors haven't thought of".

@buschtoens
Copy link
Contributor

buschtoens commented Aug 1, 2017

Aria attributes are a web standard like any other, and most importantly Ember as an ecosystem is about good defaults for letting you use web standards to make great experiences for your users. Making accessibility hard doesn't seem like a good default.

Please don't get me wrong. I'm not saying it isn't a standard or that we should make accessibility intentionally or inadvertently hard. Quite the contrary actually. 😄

I think that's more of a strawman. aria attributes are a lot more common than you'd think and more generically applicable. I think that's why this RFC is good! It limits us to just ARIA things.

I only wanted to point out that this could set a precedent for letting "non-primitive" features creep into the framework API and users could bring this up when asking for more default attribute bindings.

If the primitive is non-accessible to Ember developers now or in the immediate future, what should people do?

This use case is solvable in both curly components and glimmer; I don't see why users should have to pick one.

Valid point.

Since Glimmer components are a distant future and performance is likely no problem, feature creep and increased API surface area are my only concerns.

They have been addressed and if the core team thinks that the benefits outweigh, I am of course on board with that decision.

Ideally we would converge on a primitive that would work in the near future, like what @mmun suggested.

@rtablada
Copy link
Contributor

rtablada commented Aug 2, 2017

While I like @mmun’s recommendation for a new helper I think it deserves a separate RFC and IMO would be a great addition.

That said, I agree a lot with the reasoning that Ember should put A11y at the forefront of features and basic support around the most common aria roles shouldn’t be “oh use the mixin or helper for non-standard HTML attributes”.

I was trying to address that with my remarks on inheriting from a common AccessibleComponent

This RFC begins to push that every component should start to be an “AccessibleComponent” or at least a “SlightlyMoreAccessibleComponent”.

Especially as a newcomer to web development or Ember.js chances are that you've never heard of aria-*.

While I think that a lot of jr devs may not be familiar, not supporting aria is actually making that worse since it’s an extra hurdle for them to go through.
From teaching boot camp developers I can say first hand that extra complexity (adding a mixin or inheriting from a different component) creates a barrier most devs won’t jump over.

Since a growing number of devs coming to Ember from other ecosystems value presentational/template only components I think it’s valuable that we don’t require them to suddenly modify/create the JS for binding aria values.

@MelSumner
Copy link
Contributor Author

It's appropriate for Ember.component to have the role attribute available (through ariaRole), because the native roles for landmark regions are only automatically attributed by assistive technology when those elements (header, footer, etc) are a direct descendant of the body tag. Since Ember wraps everything in a div, the roles have to be added to those elements. (And, there's an addon for that, of course! see ember-a11y-landmarks in the Ember A11y github repo).

It's perhaps not the wisest idea to give false equivalency to "all" Ember developers, or even "most" Ember developers, just by looking at who is in Slack or comments on Github. Ember is for building "ambitious applications," right? Consider all of the enterprise developers who use Ember, who are not permitted to use Slack on their computers at work, or not permitted to use Github for their code. My perspective is one of an enterprise developer. There are a lot of us, and we do have a legal requirement in the US for accessibility, and it effects our jobs every day. We contribute to open source in other ways, some of us during nights and weekends.

The U.S. isn't the only country that requires digital accessibility for specific "places of public accommodation." Similar laws exist around the world - the places I know of off the top of my head are the UK, Sweden, Japan, and Spain.

I've spent the last year of my life, focusing on accessibility as a singular focus. I've read through every page of the ARIA spec, looking at all of the methods for success and failure, and trying to figure out how to make it simple enough so other developers could reasonably care, but still leaving it complex enough so it's not too boring. I'll be honest, I haven't found that balance quite yet. But since 1 in 10 users has some sort of barrier that effects their relationship with technology, every bit helps.

The idea that @mmun presented is intriguing. I like it because it thinks bigger than just what I was asking for. However, when considering what to ask for in this RFC, I thought about the things I needed and the things I wanted, then applied the constraints of "keep it specific." It's more likely to be adopted in the enterprise world if it's specific, because specificity provides guidance.

@mmun
Copy link
Member

mmun commented Aug 2, 2017

@MelSumner

It's more likely to be adopted in the enterprise world if it's specific, because specificity provides guidance.

I strongly agree with this statement. However I've been using Ember for 4+ years and I didn't know there was an ariaRole property on components. I would like to see a section in the guides that explains (the basics) of making accessible components.

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2017

The idea that @mmun presented is intriguing. I like it because it thinks bigger than just what I was asking for. However, when considering what to ask for in this RFC, I thought about the things I needed and the things I wanted, then applied the constraints of "keep it specific." It's more likely to be adopted in the enterprise world if it's specific, because specificity provides guidance.

Sorry to be "that jerk", but I actually disagree fairly strongly here. It is (and as far as I can tell always has been) Ember's job as a framework to provide general solutions to common problems. The somewhat narrow and specific problem that this RFC is attempting to address is a few ARIA attributes, but from my perspective that is only a very very tiny drop in the bucket.

We need to write idiomatic standards compliant HTML which means that we will need MANY attributes all based on how we are actually using an underlying component. Solving the very specific problem of these 3 attributes, doesn't help us at all with solving the others (and adds even more to the "Ember is too magical" camp).

Basically, I'm saying that I think the answer to this question:

How do I add XYZ attribute to the root element of a component that I'm invoking?

Should not involve memorizing a mapping of attribute to property names. Every rule we add to this mental mapping, makes it less likely that any one single rule will be remembered ("brain buffer overflow" 😝).

I think we should absolutely try to push forward on @mmun's suggestion.

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2017

From @mmun in #242 (comment):

I would like to see a section in the guides that explains (the basics) of making accessible components.

Totally agreed!

@MelSumner
Copy link
Contributor Author

Sorry to be "that jerk", but I actually disagree fairly strongly here. It is (and as far as I can tell always has been) Ember's job as a framework to provide general solutions to common problems. The somewhat narrow and specific problem that this RFC is attempting to address is a few ARIA attributes, but from my perspective that is only a very very tiny drop in the bucket.

I don't think you're being a jerk at all. I think we agree on the heart of the issue. I could have better explained my position by giving an analogy- providing a machine-readable name is like filling up the bucket a third of the way. It's not the only thing you have to do, but it's a huge step in the right direction.

I was asking for the base model car, @mmun approach gives me the car + all the luxury features for free. More than I was targeting, so if it can be implemented and makes more sense to implement, I'm all for that.

@rtablada
Copy link
Contributor

rtablada commented Aug 2, 2017

@rwjblue I think a good spot would be let's grab some of the A11Y specific engineers from the bigger Ember teams and have them weigh in to get this RFC to the most vital accessibility attributes (maybe that discussion boils down to ariaRole and that's it, maybe more).

I think let's start as a separate thread/RFC the html-attributes helper and get that moving.

I think we've seen three different things that are three different problems here:

  1. How do we make it easier and pragmatic to create Accessible components with attribute bindings
  2. How do we have binding for other attributes (@mmun's suggestion)
  3. Glimmer components already solve this (but as @fivetanley points out, this doesn't solve traditional component use and is a ways out)

@Robdel12
Copy link

Robdel12 commented Aug 2, 2017

Pinging @krivaten :)

@mmun
Copy link
Member

mmun commented Aug 2, 2017

Hey folks. @rwjblue and I spiked out an addon for #242 (comment).

Check out https://github.com/mmun/ember-component-attributes.

If anyone is up for writing a new RFC based on this addon I'd appreciate it :)

@crodriguez1a
Copy link

crodriguez1a commented Aug 2, 2017

@mmun this is a very elegant solution. Any reason for not going with the html-attribute naming as mentioned before? attribute feels a little less specific. 🙂

@mmun
Copy link
Member

mmun commented Aug 2, 2017

@crodriguez1a I'm not totally sold on it either. I went with attributes because it's shorter and felt it was clear enough. I'm happy to change the name if we can get community consensus on a better one. Feel free to make an issue in the addon's repo.

@crodriguez1a
Copy link

@mmun #2

@MelSumner
Copy link
Contributor Author

MelSumner commented Jun 1, 2018

Closing this.

@MelSumner MelSumner closed this Jun 1, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.