whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.13k stars 2.67k forks source link

Move the focus back to the previous focused element for dialog.close() #5678

Closed sefeng211 closed 3 years ago

sefeng211 commented 4 years ago

From an accessibility pointer of view, dialog.close() should move the focus back to the previously focused element to provide good accessibility.

domenic commented 4 years ago

What is "the previously focused element"?

sefeng211 commented 4 years ago

I mean the element that was focused before the dialog showed up.

domenic commented 4 years ago

What if that element has been made un-focusable (removed, made inert, etc.)? What about if it has moved to another location or reused for a different purpose entirely (e.g. via a virtual DOM framework)?

In general, this would be the first time that the specification focuses an element without explicit web developer request or user interaction, so it raises all sorts of unprecedented questions.

annevk commented 4 years ago

Using https://software.hixie.ch/utilities/js/live-dom-viewer/ I played with variants on <input oninput="alert(1); document.body.appendChild(this); w(document.activeElement);"> to figure out what might be reasonable solutions to those problems. It seems that in general browsers fallback to the body element (the document in the model, iirc).

And is it really that different from showing the dialog running focusing steps? (Note that I'm not sure this should be limited to close(). It seems like something that would happen for dismissal in general.)

MarcoZehe commented 4 years ago

Yes, it is true that this re-focusing of the element that triggered the dialog is true for any kind of closing the dialog. See also the WAI-ARIA design pattern for role "dialog". It could be argued that dialog is a complex enough compound widget, with modality, anyway, so this is the first of its kind in the HTML specification, and it would also be the first of its kind to request that focus be set back to the triggering element if still possible. The alternative would be the body element, which would require keyboard users to tab a cillion times to get back to the element that triggered the dialog, and completely lose context in the meantime. It would eliminate a very common accessibility mistake if browsers would do this automatically if the triggering element is still focusable.

annevk commented 4 years ago

As for implementer interest, Mozilla is interested.

@hober @cookiecrook there was some talk about WebKit eventually adopting <dialog> too, would you all like to see this changed too?

@mfreed7 what about Chrome?

cookiecrook commented 4 years ago

It's always been recommended that authors move focus back to the "trigger" element (the element whose action caused the dialog or menu to be displayed). This is is consistent with platform conventions, too. For example, on popup buttons like <select>, the focus is moved back to the button after a selection has been made (Space/Enter/Click), or when the menu has been dismissed (Escape).

However, perhaps it's too simple to spec the "trigger" as the "previously focused element"? I think most of the time, the previously focused element is the same as the trigger, but there may be some scenarios where it's not. For example, if something is clicked and doesn't received focus from the click, then the "previously focused element" would be whatever was focused before the click. I don't know how common it would be. I'm just trying to consider all scenarios.

cookiecrook commented 4 years ago

As for WebKit's opinion, there has been support expressed for the inert attribute but <dialog> is still under consideration.

cookiecrook commented 4 years ago

(Updated: Re-posting this musing as a standalone comment to make it clear it's not an official WebKit position.)

In line with that, perhaps this "focus the previously focused element" idea should not be tied to the <dialog> element at all. It seems like a focus stack could be associated with the inertness primitive. A lot of JavaScript frameworks implement focus container stacks (I've helped with a few) so there is definitely a common use case outside <dialog>.

annevk commented 4 years ago

https://github.com/whatwg/html/issues/4937#issuecomment-536806327 is what I was referring to above with regards to interest from WebKit.

cookiecrook commented 4 years ago

IIRC, there were some outstanding issues with dialog positioning yet to be addressed by CSS WG. @smfr may recall if there are any remaining blockers.

hober commented 4 years ago

You're probably thinking of w3c/csswg-drafts#4645, @cookiecrook.

Yay295 commented 4 years ago

For what it's worth, this is how the jQuery UI Dialog works.

Upon closing a dialog, focus is automatically returned to the element that had focus when the dialog was opened.

- https://api.jqueryui.com/dialog/

The jQuery UI Dialog also keeps focus within itself, so you can't tab out of it to something in the background (and if it's a modal dialog you also can't click out of it).

annevk commented 4 years ago

@mfreed7 @domenic ping. This is a blocking issue for Firefox and it'd be great to make some progress.

domenic commented 4 years ago

I remain weakly opposed to this for the reasons stated above, but @mfreed7 would be the ultimate implementer. I think it would be ideal if Firefox produced a proposed processing model and tests to answer about how the various edge cases I mentioned are handled.

alice commented 4 years ago

It seems like a focus stack could be associated with the inertness primitive. A lot of JavaScript frameworks implement focus container stacks (I've helped with a few) so there is definitely a common use case outside

.

I generally agree, but I would tie it to the "modal top layer" idea rather than inertness.

There was some discussion of this in this thread from 2016: https://github.com/whatwg/html/issues/897 (later in that thread)

That also points to a since-deleted "focus fixup rule 3" - does anyone know what happened to that?

annevk commented 4 years ago

See a4bb3465c86896b95b7d66c1e9047e8a313c7212 (and also 90a60b2a0dc740b8b0093b07ca0a41e70ba8d83a). A bunch of focus logic around the dialog element was removed in 2018 due to Chrome not implementing it and nobody else voicing a strong opinion.

mfreed7 commented 4 years ago

I am supportive of the general fix here - to return the focus to a useful spot, rather than <body>, when the dialog closes. Having said that, I don’t know how to define it, and the questions raised by @domenic need to be answered. Assuming reasonable answers there, I think we would support this.

annevk commented 4 years ago

@cookiecrook @alice does either of you expect to make progress on a new focus concept around inert/top layer? Otherwise, it seems like a simple and effective way forward here would be to record the previously focused element on the dialog element itself and restore focus there (if still focusable) once the dialog is closed. A broader version of focus scoping can maybe build on top of that in the future?

cookiecrook commented 4 years ago

@cookiecrook @alice does either of you expect to make progress on a new focus concept around inert/top layer?

I don't have any short terms plans to work on that.

Otherwise, it seems like a simple and effective way forward here would be to record the previously focused element on the dialog element itself and restore focus there (if still focusable) once the dialog is closed. A broader version of focus scoping can maybe build on top of that in the future?

I don't see a problem with this approach being the default behavior for dialog. Seems reasonably forward-compatible to me, even if it's reopened for improvement later via #897 or otherwise.

I'm speculating that there may be a timing issue though… If the "trigger" is inert (or otherwise disabled/inactive) while dialog is displayed, how long should the UA give the author to re-enable it (timeout for attempted re-focus?) or should the autofocus behavior be limited to UAs only?

melanierichards commented 4 years ago

Assuming reasonable answers there, I think we would support this.

Just wanted to +1 on general implementer interest from another Chromium contributor (MS Edge). Providing focus rewind as a default—with simple author control if a different experience is required—seems likes a win for both authors and users assuming we can work around the identified issues.

alice commented 4 years ago

https://github.com/whatwg/html/issues/4633 began a discussion around top layer APIs, I'm not sure whether it was continued elsewhere.

I would love to eventually figure out the interactions between inert, top layer, dialog, blocking elements, and focus fixup/replaying.

In the meantime I agree with @annevk and @cookiecrook that we could specify something sensible for dialog and generalise it as we go.

I like the idea of restoring focus to the previously focused element, although it's worth noting that in browsers which don't always focus elements on click (i.e. browsers not based on chromium), the previously focused element may not be the element that triggered the dialog. Perhaps we should also specify that we should not scroll to the element that gets focused as a result of a dialog closing, to avoid a confusing experience in that case.

I'm not sure I understand the timing issue you refer to though, James. If we're talking about steps for hiding a dialog, won't the UA be doing the focusing? And if we're not talking about that, what are we talking about?

sefeng211 commented 4 years ago

Gecko's work will be tracked at https://bugzilla.mozilla.org/show_bug.cgi?id=1660271.

We'll start with By default, dialog.close() returns the focus to the previously focused element if the element is still focusable and within the viewport, otherwise move the focus to <body>;

I think what James' concern was, how do the authors move the focus upon modal dialog closing because the element that they want to move could be in inert state? I think we can solve it by guaranteeing that the document is no longer blocked by modal dialog before the close event is fired?

So the order of steps is Unblock document -> move focus to previously focused element -> dispatch close event. Then using the close event can move the focus to a different element.

If I am missing something, please let me know!

cookiecrook commented 4 years ago

Alice wrote:

I'm not sure I understand the timing issue you refer to though, James. If we're talking about steps for hiding a dialog, won't the UA be doing the focusing? And if we're not talking about that, what are we talking about?

That's what I was trying to clarify with the comment "or should the autofocus behavior be limited to UAs only?" I don't think there will be an issue if inertness and auto-re-focus behavior is limited to UAs only.

I was thinking of some hypothetical scenario where the author also modified the DOM at the time that the dialog was presented (a DOM change that might break the auto-re-focus behavior). Should the UA provide some non-blocking way to do the focusing after the author has had a chance to do any necessary DOM fix-up at the time of the dialog close?

It's probably a non-issue that can be addressed when/if a more concrete example arises. In the meantime, authors should probably achieve this by performing any necessary DOM fix-up prior to dialog.close()

cookiecrook commented 4 years ago

@sefeng211

So the order of steps is Unblock document -> move focus to previously focused element -> dispatch close event. Then using the close event can move the focus to a different element.

That order may be enough... but the duplicate focus events in quick succession would potentially be noticed by AT users.

The hypothetical scenario I was thinking of might be more like this:

  1. Unblock document -> [dispatch onbeforeclose event allowing author to fix-up and potentially focus] -> move focus to previously focused element [unless the author moved focus already] -> dispatch close event, or…
  2. Unblock document -> dispatch close event allowing author to fix-up and potentially focus -> move focus to previously focused element [unless the author moved focus already as part of the close event handler]
sefeng211 commented 4 years ago

@cookiecrook

That order may be enough... but the duplicate focus events in quick succession would potentially be noticed by AT users.

Yeah, fair point, we don't want to move the focus twice.

Unblock document -> [dispatch onbeforeclose event allowing author to fix-up and potentially focus] -> move focus to previously focused element [unless the author moved focus already] -> dispatch close event, or…

It introduces a new event which we hope that we can avoid.

Unblock document -> dispatch close event allowing author to fix-up and potentially focus -> move focus to previously focused element [unless the author moved focus already as part of the close event handler]

The issue is the close event is an async event, it's going to be awkward that we need to wait for an async event before continuing the algorithm.

Can we dispatch the close event as a sync event, then we can rely on the close event to determine whether we need to move the focus to a different element? It's not very clear why the close event is async at the moment. The document is already unblocked and plenty of JS may run before the close event is dispatched. And mousedown/up events could easily get through before close fires. However, making this change requires blink to change what it's already shipped.

@alice @mfreed7 What do you think the above changing for close event?

cookiecrook commented 4 years ago

cc @smfr

annevk commented 4 years ago

@alice @mfreed7 @cookiecrook @smfr ping. Would be nice to make progress here!

rniwa commented 4 years ago

It's very strange that close event is async. Since we're already mutating the content attribute which would cause mutation events to fire in all major browsers, there doesn't seem to be any security / integrity benefit either.

sefeng211 commented 3 years ago

@alice @mfreed7 @smfr Do you think changing the close event to a sync event to allow authors to change the focus is a reasonable approach here? I am happy to propose the spec changes.

josepharhar commented 3 years ago

It doesn’t sound like anyone is opposing making the close event sync instead of async, so I suppose I could implement it in chrome along with the other focus change proposed here.

I don’t know why it is async and I don’t know if changing it will break any websites, but I could say the same thing about the proposed focus change and I won’t be sure until I try.

Are there any WPTs for this behavior yet?

domenic commented 3 years ago

The usual danger with sync events is outlined in https://w3ctag.github.io/design-principles/#guard-against-recursion . I'm not sure whether any of that reasoning applies here; I think it would require code like dialog.onclose = () => { dialog.open(); dialog.close(); } which is probably not something we really need to guard against.

annevk commented 3 years ago

(There is a small risk with regards to eventually removing support for mutation events (or changing when they fire, at least), but if we manage that we could certainly move this to CEReactions timing then too, I'd think.)

josepharhar commented 3 years ago

While implementing this in chrome, it was pointed out that changing focus synchronously, which fires an event, will make us have to write the code somewhat carefully (making sure that changing focus is the very last thing we do).

Was it ever considered to change focus asynchronously, or would that break anything? Am I missing anything on this topic?

annevk commented 3 years ago

Is it materially different from calling focus()? Given that screen reader experience is defined by what has focus, keeping the focus on a closed dialog might not yield the best results, I'd think.

AutoSponge commented 3 years ago

The intent to manage focus of a closing dialog is a good one. However, if authors have a clear understanding of the process they should have the ability to change the focus target on dialog.close().

A common example: a dialog created for the "confirm delete" action with "confirm" and "cancel" buttons. Cancel can target the default element per this spec (i.e., the "delete" button). Confirm would send focus to body since the deleted item no longer exists. An author should have the ability to override this behavior but programmatically setting focus could introduce a race condition with the default behavior.

Possible solutions:

annevk commented 3 years ago

@AutoSponge thank you! Could you file that as a new issue please? Comments in closed issues tend to get lost.