aurelia / dialog

A dialog plugin for Aurelia.
MIT License
106 stars 115 forks source link

Focus management on open / close #375

Closed RomkeVdMeulen closed 4 years ago

RomkeVdMeulen commented 5 years ago

I've been looking at the accessibility of the dialogs, notably whether they comply with the WAI-ARIA best practices for dialogs. The native renderer introduced in https://github.com/aurelia/dialog/pull/338 helped a lot. Notably, it ensures focus is placed on the dialog when it opens and cannot leave the dialog. Perhaps similar functions should be added to the standard renderer to make it properly accessible, but that's not what I'm here for.

One accessibility problem remains and that is that focus is not properly returned to the underlying document when the dialog closes. This can make it very difficult to navigate using only a keyboard, and can result in very unclear situations for screenreaders.

The solution is pretty straight forward: the last focused element at the moment a dialog opens needs to be remembered, and focus should be returned to this element when it closes. If that element is no longer available, focus needs to be returned somewhere else in the document. This process needs to be overridable for specific dialogs, since the user may know a better place to return focus to once the dialog closes. See the WAI-ARIA notes as well.

StrahilKazlachev commented 5 years ago

So, I'll try to be as structured as possible(all is personal opinion):

  1. About the concerns of this issue: 1.1 About not allowing focus to leave the dialog, I don't agree that it should be provided by the dialog. A complete cross-platform tab trap implementation seems quite the challenge. If there is some library that does this then wrapping it in custom attribute seems way more logical. 1.2 Capturing and restoring a focused element seems to be one of the appropriate things to be done in a Renderer, and it would be nice if it can be done as a reusable logic that other renderers can utilize. Also it must be taken into consideration any possible implications of having multiple open dialogs.
  2. Current weak points of the dialog: 2.1 The Rederer concept does not seem well defined, it seems that it's only concern should be attaching/detaching, but it is used for other purposes as well, like adding common dialog elements(backdrop, etc.), common behaviors(keyboard, mouse). This behaviors are internal impls of the default renderer, no other renderer can just plug them in(I don't mean inheritance). Also there are some settings in DialogSettings concerning them, so does this make them mandatory for every renderer? If it is, does adding a new setting(about locating a fallback element to focus) make other custom renderers "broken"? 2.2 No official way to pass custom settings - be it to the view model, Renderer, etc. 2.3 Lack of proper separation - all out-of-the-box custom elements/attributes, renderers, etc., should be in separate package/s. Currently I see this as a burden for the build setups.
RomkeVdMeulen commented 5 years ago

@timfish @bigopon Any thoughts?

bigopon commented 5 years ago

Maybe we can have an interface for this, say IFocusManager:

interface IFocusManager {
  // return element to be focused when this dialog is closed
  // return null means don't focus anything?
  onDialogOpen(lastActiveElement: HTMLElement): HTMLElement;

  // only invoked when onDialogOpen returns an element?
  // return null means don't focus anything?
  onDialogClosed(toBeFocusedElement: HTMLElement): HTMLElement;
}

Basically when dealing with form, focus strategy may need to be more than just simple last blurred first focused.

RomkeVdMeulen commented 5 years ago

@bigopon I went for a simpler interface. If a developer wants to restore focus to a different element they can simply override the restoreFocus setting and track the element themselves. The restoreFocus setting can be overriden in plugin settings or on a case-by-case basis by overriding at each DialogService.open() call.

@StrahilKazlachev I went for a simple addition to both renderers. While I agree that the code could do with some refactoring, I think it's out of scope for this bug.

RomkeVdMeulen commented 4 years ago

Fixed in https://github.com/aurelia/dialog/pull/378