New issue

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

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

Already on GitHub? Sign in to your account

Outlet Focusing #66

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@nathanhammond
Copy link

nathanhammond commented Jun 14, 2015

Rendered

Research:

Implementation:

  • Build a smarter default focus hook.
  • Better leverage Ember's lifecycle hooks so that it isn't as tightly coupled.
  • Implement opt-in to global outlet wrapping.
  • Clean up prior focused element.
  • Fix `{ path: '/' } for pivotHandler name checking.
  • Fix repeated navigation to the same route.
  • Write really weird test cases.
@Robdel12

This comment has been minimized.

Copy link

Robdel12 commented Jun 16, 2015

Absolutely 100% agree with this. I was tossing around the idea of creating an addon that handled this but this absolutely is a core feature. I love that ember is getting some A11Y support. :)

👍

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Jun 16, 2015

I am strongly 👍 on the concept here, we should definitely bake something in for this.

I am not sure that DOM focusing behavior belongs in the route (though that isn't super important for the larger purpose here).

In the mockup gist, I like that the implementation is done in the {{outlet}} helper. We can likely do something at that level fully and not involve the actual route.

@jessebeach

This comment has been minimized.

Copy link

jessebeach commented Jun 16, 2015

This is an awesome basic utility.

Programmatically setting focus is often done poorly and as a result it is generally recommended against in the assistive tech community. This behavior is far from being any kind of standard and we will be charting new waters. This is something we should take on, but we will likely get pushback on any decision that we make, either way. We should make sure we involve people outside of just the Ember community.

This has been an issue and I would expect pushback. My intuition is the pushback has been against poor implementations. I really hope that we can get to a place where focus is handled dynamically and with courtesy so that application flows are expressed seamlessly in visual and aural modalities. Keep experimenting!

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Jun 16, 2015

From experience with liquid-fire, adding toplevel divs to people's outlets is fairly disruptive.

Can we get a similar benefit by focusing the first DOM node found inside the outlet? In cases where the user has already provided a single topmost element we would find it and use it, otherwise we would at least place the focus at the start of the new content.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jun 18, 2015

Okay, so it sounds like we want to do this, as well as ship a default implementation. This is just a starting place, so much more we can do... :)

Notes from in-person conversations at Wicked Good Ember:

  • Bikeshedding: if an Ember developer is setting focus to a route's contents, the expected end-user API would appear at the route level. This makes sense as an API for now and can be aligned with something more reasonable post-routable-components (@rwjblue, @ebryn)
  • This implementation focuses the first element that is a descendant from the (inferred) primary {{outlet}}. This allows for opt-in behavior (wrap it yourself) without a breaking change at the Ember outlet level. I'd like the "wrap outlet in div" to be an app-level configurable behavior. (@ef4)
  • This implementation does correctly set focus at your ideal level, without cascading. (@stefanpenner)
  • It's definitely possible to improve the way I wire the route and transition together through reliance on typical Ember patterns and knowing where in the lifecycle certain things occur. This implementation is basically an explicit bind to a particular timeline. (@stefanpenner)

@jessebeach I'm going to reach out to you on a different channel, I feel like we should probably have a conversation. :)

Adding a to-do list to this RFC.

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Jun 18, 2015

I'd like the "wrap outlet in div" to be an app-level configurable behavior.

I would be hesitant about that. It means you can't jump into an ember app and understand what {{outlet}} means without checking a global config first. And if it's optional, it will be unusual.

If we can't establish one behavior that works for everyone, I would rather help organizations opt into stricter behaviors through something like custom template linting rules.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jun 18, 2015

When @ef4 makes a good point... :) And since you hook into {{liquid-outlet}} at least that information is available local to use case and the person should be able to identify the consequences of wrapping.

That being said, my goal would be that (something like) this could be a breaking change in Ember 3.0, and exposed as a feature in the 2.x series via global configuration that also throws a deprecation when configured for non-wrapped outlets. Yes it would require a possibly non-mechanical upgrade, yes it requires a flag, but being inaccessible because you didn't change the way you did your {{outlet}}s seems to be a pretty large penalty.

So, with your feedback in mind, my proposal is to change the default expectation which skirts around your problem. For bonus points it makes liquid-fire a touch easier to integrate for most users as you'll get your child container by default.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jun 20, 2015

The scroll behavior can be caught and prevented for sighted users: http://jsbin.com/faxazedaci/edit?js,output

However, the function that is called for this handler should probably also be exposed as a public API as it is variable which element is scrolling, and it's a common enough problem that we have a guide for it: Resetting scroll on route changes.

I feel like the open questions are resolved at this point, final public API needs to be discussed by core team, and this RFC needs to be updated to reflect those opinions. Unsurprisingly, I volunteer to implement. :)

/cc @drewlee @trentmwillis

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Jun 22, 2015

Morphs are not a public API, so you will need to design an alternate API that does not expose them to application developers.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jun 23, 2015

@tomdale My goal was pretty simple, I wanted to provide a maximally flexible primitive for extensibility. I know for a fact that we can't anticipate every use case (especially for something as important as designing an aural experience with inconsistent user agents) and I wanted to expose the possibilities of the underlying platform as closely as possible. (Yes, I'm quoting dogma.) It's for that reason I selected pushing the Morph all the way into a user-facing API. Below you'll find an enumeration of everything I considered.

Do you have additional proposals for focus behavior that don't expose the Morph and don't come with some of the drawbacks of the first two? I'd even be game for a layer of indirection (enter hook versus activate hook for example) which pushes the default API up the ladder of abstraction while still giving you the ability to monkeypatch your way to success.


Declarative in Template

In this case you could allow a user to declare which element is to be focused inside of their templates. To do this well we would need to ensure that every possible permutation of template content has exactly one focus target.

Benefits

  • Simple.
  • Easy to understand.
  • Pattern is familiar and feels Ember idiomatic.

Drawbacks

  • Easy to screw up your template or have it drift.
  • Multiple "focus" attributes in the template results in implementation-defined behavior (breadth-first search, presumably).
  • Doesn't make it as easy to expose making this choice to programmatic JavaScript analysis of state (computed properties).
  • To make this accessible by default when the user doesn't add a focus attribute it's expensive, performance-wise. (Traverse "own nodes" and then set to first node.)
  • If we're trying to throw focus to a wrapper that is always inserted around an outlet's content (and doesn't appear in the template–one of the things I'm proposing) we trigger that worst-case behavior every time.

No Choices

We force all outlets to be wrapped in a div (either from Ember itself, or as the first node in your template to opt in), and that's what is focused.

Benefits

  • Dead simple.
  • Easy to explain.

Drawbacks

  • Assumes that we know everything about every use case.
  • Assumes that all screen readers have no bugs that need to be worked around.
  • Doesn't give the user any choice for designing their user experience.
  • No backwards-compatible hook or opt-in point if you already wrapped your outlet in a div, making it hard to land.

Exposing the Morph

Moves the focusing logic out of declarative code and into imperative code with a reasonable default implementation so that most people never touch the Morph that gets passed in.

Benefits

  • Maximally flexible.
  • Allows us to code our way out of any corner–without waiting on Ember to upgrade.
  • Relatively easy to explain to any Ember developer.

Drawbacks

  • Exposes a low-level feature of the framework as API that may need to be understood.
  • Doesn't feel as Ember idiomatic the moment you have to override the focus hook because you have to start paying attention to DOM nodes.
@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jul 3, 2015

I missed the obvious solution: allow a user to pass in a selector in public API space, create a private API space function that receives the morph, grabs the selector, and sets the focus. Will update the RFC to be a complete picture of my current proposal with hopes to move this RFC into an "active" state.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Jul 3, 2015

@nathanhammond Nice, looking forward to it

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Jul 3, 2015

I've updated the RFC, please re-review. Major differences:

  • Ember.Route.focusNode property which is a CSS selector.
  • Ember.Route._focus(morph) function which handles the process of setting the focus to the focusNode.
  • focus event fired at a single route per transition (distinct from enter and activate) which can be used for additional behavior (the focus pivot–different from the pivotHandler).
  • Deprecation of non-div-wrapped {{outlet}}s.

I expect the last of these four to be the most contentious item in the RFC (it's the only breaking change), but it guarantees so much better results for assistive tech users that anybody who is serious about accessibility should be doing it anyway. Making this change will make it very uncommon that overwriting the default gives assistive tech users a better experience and all Ember apps will benefit. (Or, phrasing differently, making this change means that by default assistive tech users will likely have the best experience possible on every Ember app they encounter.)

@backspace

This comment has been minimized.

Copy link

backspace commented Aug 17, 2015

Thanks for your work on this, @nathanhammond. Is this RFC languishing? I think it would be an important statement to make consideration of accessibility a central goal of Ember, above just making Ember-developed sites more accessible by default.

@jennisongithub

This comment has been minimized.

Copy link

jennisongithub commented Aug 26, 2015

Stepping into this conversation wearing my hat as owner of accessibility at LinkedIn (and a native screen reader user) to ask the community for its support in doing what is necessary to assist @nathanhammond to quickly move this RFC through your approval process because the issue it seeks to address currently represents an accessibility show-stopper for anyone, including us, who want to develop apps using Ember that can be used by everyone, including screen reader users.

I am unsure how your approval process works. That said, if there are any remaining concerns, could these please be identified now so Nathan can address them? Otherwise, I am asking the community to see your way to approving this RFC without delay so that it can be implemented. Based on feedback at Wicked Good Ember, I was pleased at the overwelming desire to make Ember-built apps accessible. Getting this critical issue resolved will go a long way in demonstrating this aspiration. @tomdale @wycats @rwjblue

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Aug 26, 2015

This is a thorny problem due to how apparently terrible screen readers are at reacting to DOM mutation.

I am hesitant to make architectural changes -- especially ones with cost to the existing user base, like deprecating non-wrapped outlets -- that are based not on actual standards for accessibility, but on de-facto accessibility tricks that could break tomorrow, as browsers and screen readers change.

The solution is also very incomplete, given how many other ways an Ember app will choose to mutate DOM:

{{#if myStuffIsStillLoading}}
  Hang on
{{else}}
  The stuff ....
{{/if}}

That would get missed by the focusing solution. Another problem scenario is anyone with multiple outlets, or with a more 2.0-style component-plus-service architecture for a sidebar or topbar. Lots of critical content in an Ember app is going to change outside of outlets. That is an encouraged and supported way to do things.

The W3C does have a standard that looks like it solves our problem. I realize that user agents have been slow to fully implement it, but we can actually help push them forward by building it into every Ember app by default. The more apps we ship that use aria-live and friends, the more valuable implementing those features becomes to the browser vendors.

@Robdel12

This comment has been minimized.

Copy link

Robdel12 commented Aug 26, 2015

I currently use aria-live as an intermediate step. But I feel like that's a hack more than the solution. But right now anything is better than nothing :)

@jessebeach

This comment has been minimized.

Copy link

jessebeach commented Aug 26, 2015

As I understand the discussion, the focus management is being proposed to solve two use cases:

  1. A UI flow is loaded the requires constrained interactivity, for example, a dialog.
  2. New content is loaded on the page and this app state change is only expressed visually.

In the first case, the constrained flow case, focus management is absolutely necessary. It's jarring either not have the focused moved into the dialog or not have the focused returned to the source element that started the flow. But this issue can be narrowly constrained to dialog interactions, at least for the purpose of this problem. As long as the routing system supports the control, even if the controls are arcane, this behavior can be built into component libraries so that product devs dont' have to worry about the specifics; they just use the dialog component.

For case 2, the loading of new content, I agree with @ef4 that aria-live is the best solution. We don't want to skip focus around the page as new content loads. AT vendors cite low adoption rates as a reason not to support features like aria-live. The more we use them, the less weight this argument has. @mcking65 is an ARIA spec author; he's pushing on AT vendors for better support of these features. Being able to cite Ember has an implementing framework bolsters that argument.

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Aug 26, 2015

I just recreated a demo like @nathanhammond's original from Wicked Good Ember and tried putting aria-live="polite" at the top of the application. It works just as well as the focus solution in Safari with VoiceOver, as far as I can tell.

@Robdel12 Why do you think aria-live is a hack? It was literally created to solve this case: telling assistive technologies that a part of your page is dynamic.

@jennisongithub

This comment has been minimized.

Copy link

jennisongithub commented Aug 26, 2015

@ef4 is it possible to share a URL so that your demo can be tried with JAWS and NVDA as well? Following standards notwithstanding, what ever solution(s) is ultimately agreed on and moved forward with, it will need to stand the test of different screen reader+browser combinations. The reality, as much as some hate it is that the balance of average screen reader users use either IE or FF with JAWS, based on the WebAIM screen reader survey, which is all we have re data. This is certainly the case based on the LinkedIn members I have interacted with.

I appreciate that this conversation is moving.

@jessebeach

This comment has been minimized.

Copy link

jessebeach commented Aug 26, 2015

@ef4 As a demo, it's illustrative. But just be wary of apps with lots of subtly changing nodes. The "time since" auto-updates are particularly nuisant. Your app ends up spewing endless "5 minutes ago...6 minutes ago" updates into the audio stream. We have a similar issue with a role=log element in a chat app and from personal experience, you want to throw the computer across the room after a few minutes of these.

@Robdel12

This comment has been minimized.

Copy link

Robdel12 commented Aug 26, 2015

@ef4 Basically @jessebeach summed it up very well. Anytime a DOM node changes it's going to alert the screen reader and it will begin to read it off. For example if github used aria-live in this comment thread, then every minute it would read off the time change from each comment.

That's why I think it's a hack to throw aria-live on it. It works in my case but it won't work for other ember apps. So I don't think aria-live is our solution.

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Aug 26, 2015

Yes, we need to try to automatically balance "too quiet" vs "too loud". The present situation is too quiet. I think outlet focusing would still be too quiet, for the reasons in my earlier comment (too much real content will get missed if we only notify the user agent immediately at outlet transitions).

I agree that putting aria-live at the top of the whole application is too loud. A better solution would add aria-live only to parts of the page that are currently changing due to user input. Experimentation is needed to see how far we can go with that.

Regarding poor browser support: somebody has to implement this stuff before it's well-supported, otherwise it will never be well-supported. If we're going to also add hacks for noncompliant browsers, that is cool with me, as long as we can keep them self-contained and only apply them when we detect one of those browsers.

@ef4

This comment has been minimized.

Copy link
Contributor

ef4 commented Aug 26, 2015

I will work with Nathan to get some demos up that people can look at in different browsers.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Sep 2, 2015

Thank you everybody for your thoughts. I'm going to drop a few notes in this forum and then disappear into a back channel to work toward a solution with @ef4. Many of these things have already been said, but I'll try again in an attempt to distill them further.

Reasoning

One of Ember's underlying principles with regards to URLs was "don't break the web." We've succeeded wildly with URL management to the point that everybody knocks off the Ember router. However, the default behavior in screen readers when you visit a new page on the web is that it starts reading from the top of the newly loaded page. We've broken the web for the portion of assistive tech users reliant upon this behavior.

Problem

Ember's route transitions (equivalent to page transitions) don't trigger the reading behavior of loading a new page and, as a result, the application state changes upon clicking a {{#link-to}} helper or programmatically triggering a transition via transitionTo and friends is completely transparent to those users.

Solution

This solution is intentionally very narrowly scoped to address the one problem which Ember itself created by getting rid of full-page refreshes. Content on the page which dynamically updates being presented to users of assistive tech is an open and thorny problem. This RFC does not attempt to address that problem. I'm of the opinion that holding up a solution to a problem which we created while looking for a silver bullet to address all of dynamic content is over-engineering.

That being said, a generic solution for notifying of specific dynamic content changes is one of the open research projects on my plate. It is separate from this behavior as identifying how and when to present dynamic page content updates is about designing an experience for users of assistive tech and is likely an application-level concern which framework code should simply support mechanisms to make it easy. (I'm calling it "stream hijacking," and the premise is double-binding a stream into an off-screen notification center which can be read top to bottom like a log.)

Standards

aria-live leaves a tremendous amount to be desired. The entirety of that spec and its associated practices fit on one screen each. It's a black box of unknown behavior exactly like we were railing against with the Extensible Web Manifesto. The docs on MDN have multiple TBDs and research projects associated with it because it's so poorly implemented and used.

Further, as described by multiple people above, aria-live has what I like to describe as a "peeing section in the pool" problem. In order to fully utilize it, you have to introspect into everything it wraps in order to make sure that it all behaves correctly (see: halting problem). It works for trivial examples but very rapidly falls apart in practice. Further, aria-live is not designed to play nicely with multiple occurrences on a single page.

In short, we can't rely on behavior provided by the standards when the standards are insufficiently expressive to accomplish our goals. Insufficient standards are why we write SCSS and ES6 and have transpilers. It's why we write the Extensible Web Manifesto. We need to approach accessibility in the same way (pragmatic, and without waiting on a slow standards process) so that we don't leave today's assistive tech users behind.

@nathanhammond

This comment has been minimized.

Copy link

nathanhammond commented Feb 28, 2016

Update: Implemented as an addon, ember-a11y. Thanks to everybody here and @binhums for helping bring that home.

It's not perfect (yet), uses @ef4's get-outlet-state from liquid-fire without actually learning how it works, hasn't addressed the entire user experience for assistive tech users, but it's shippable without core modifications.

I'm closing this PR as a "wontfix." As ember-a11y matures I'll instead push for the addon to be included in the default blueprint.

@mixonic

This comment has been minimized.

Copy link
Member

mixonic commented Feb 28, 2016

👏 Thanks for the update @nathanhammond! When we next revisit outlets (around routable components) we should remember to consider these use-cases.

@nathanhammond

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment