Skip to content
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

Focus management on open / close #375

Closed
RomkeVdMeulen opened this issue Jul 5, 2019 · 5 comments

Comments

@RomkeVdMeulen
Copy link
Contributor

commented Jul 5, 2019

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 #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

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@timfish @bigopon Any thoughts?

@bigopon

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Fixed in #378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.