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

fix a11y issue with dialog element keyframes animation #36167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clhenrick
Copy link

@clhenrick clhenrick commented Oct 2, 2024

Changing the display property in the keyframes animation causes focus to be dropped onto the body element when opening or closing the dialog element. This is harmful for accessibility, since a keyboard and/or screen reader user may become confused when focus is not managed correctly for the dialog element when used as a modal dialog. The dialog may still be animated using CSS keyframes without changing the value of the display property.

Description

Removes usage of the display property from the dialog keyframes animation example.

Motivation

During testing the dialog for accessibility purposes I saw that focus was being dropped on the body element when animating the dialog using this method. Removing the display property solves the issue and allows the dialog to manage focus correctly.

Additional details

Tested proposed changes on

  • Chrome (version 129.0.6668.70 (Official Build) (arm64)) on MacOS (14.6.1 (23G93))
  • Firefox (version 131.0 (aarch64)) on MacOS (14.6.1 (23G93))
  • Safari (version 18.0 (19619.1.26.111.10, 19619) on MacOS (14.6.1 (23G93))
  • Edge (version 129.0.2792.65 (Official build) (64-bit)) on Windows 11
  • Chrome (version 129.0.2792.65 (Official build) (64-bit)) on Windows 11
  • Firefox (version 131.0 (64-bit)) on Windows 11

Related issues and pull requests

Changing the display property in the keyframes animation example causes focus to be dropped onto the body element when opening or closing the dialog. This is harmful for accessibility, since a keyboard and/or screen reader user may become confused when focus is not managed correctly. The dialog may still be animated using CSS keyframes without changing the value of the display property.
@clhenrick clhenrick requested a review from a team as a code owner October 2, 2024 18:21
@clhenrick clhenrick requested review from chrisdavidmills and removed request for a team October 2, 2024 18:21
@github-actions github-actions bot added Content:HTML Hypertext Markup Language docs size/xs [PR only] 0-5 LoC changed labels Oct 2, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Preview URLs

@chrisdavidmills
Copy link
Contributor

@clhenrick hi there, and thanks for contributing to MDN!

I really appreciate your concern here, but the trouble with removing display from the keyframes is that it breaks the closing fade-out animation. Plus the whole point of the example is to show animating display ;-)

I wonder if we could fix the issue presented in another way, such as by programmatically setting the focus after the anmation has finished, or by adding a note to at least warn readers of this issue?

Or maybe we should remove the animation example and just recommend using transitions for such animations instead.

I also wonder whether it might be an implementation bug,or whether it is like this by design?

@clhenrick
Copy link
Author

@chrisdavidmills thanks for the quick review and reply, happy to help contribute.

I really appreciate your concern here, but the trouble with removing display from the keyframes is that it breaks the closing fade-out animation. Plus the whole point of the example is to show animating display ;-)

I'm seeing that even when the display property is included the closing fade-out animation does not occur in Chrome or Safari. I've tried increasing the duration of the animation as well and no luck: https://codepen.io/clhenrick/pen/poMyPqa

I wonder if we could fix the issue presented in another way, such as by programmatically setting the focus after the anmation has finished, or by adding a note to at least warn readers of this issue?

Yeah we may want to warn readers that animating the display property by setting display: none will interrupt the browser's handling of focus management to the dialog and harm accessibility. But if the dialog is still animating without the use of display then I'm not sure why it's needed?

Or maybe we should remove the animation example and just recommend using transitions for such animations instead.

When looking into the various browser support for all the properties required to animate the dialog using a transition I remember there being less browser support when compared to using keyframes. For this reason in my own work I chose to use the keyframes animation method, so it seems worth keeping IMO.

I also wonder whether it might be an implementation bug,or whether it is like this by design?

I wonder this as well. I'm not seeing the closing fade out animation in Chrome or Safari on MacOS with the transition animation in the play examples on the MDN dialog page either.

@scottaohara
Copy link
Contributor

scottaohara commented Oct 4, 2024

chiming in because i helped @clhenrick review the example and noted that the fade-in animation shouldn't need display none to block at all. taking that away didn't seem to impact the animation at all (since the dialog is in a display none state by default, it immediately showing and transitioning with the opacity 0 -> seemed to work just fine). But removing that from the keyframes did solve the focus issue. (i can't imagine this focus issue being by design. likely an implementation oversight).

(fwiw, @clhenrick - the animation for the fade out works just fine for me with the display block to none. I'm testing on Windows Edge v129. Not sure what platform browser release you're using, but if you're not seeing the animation out then maybe an update is necessary?)

the fade-out animation can keep the display block to none to allow the animation to work. since focus is returning to the invoking button element, focus loss is not an issue here. So it can still be used to demonstrate the animation of display - per the intent of this demo.

It probably makes sense to add a note to indicate there's no reason for an author to re-declare a display none property for a dialog, since it implicitly is display none by default. And that doing so can result in the browser's focus algorithm for the dialog to not work, since it fires at the same time that the dialog is still considered "display none" due to the author overwriting the UA style.

@clhenrick
Copy link
Author

@scottaohara I am now seeing the fade out animation when including the display property in the @keyframes fade-out CSS block on Chrome (Version 129.0.6668.90 (Official Build) (arm64)) on MacOS (14.6.1). I'm not seeing it work on Safari (Version 18.0) on MacOS (14.6.1). Firefox doesn't support animating the dialog at all so not testing there.

I like the way Scott summarizes the issue and agree including this information in the dialog keyframes animation example would be helpful:

It probably makes sense to add a note to indicate there's no reason for an author to re-declare a display none property for a dialog, since it implicitly is display none by default. And that doing so can result in the browser's focus algorithm for the dialog to not work, since it fires at the same time that the dialog is still considered "display none" due to the author overwriting the UA style.

On a related note one thing I'm noticing is that focus management is handled correctly when animating the dialog using CSS transition on Chrome with MacOS. I'm also seeing that the ::backdrop fade out is possible with the CSS transition property but not so with the keyframes (possibly why it's not shown in the MDN article in the keyframes example). It could be worth noting this in the article as well.

@chrisdavidmills
Copy link
Contributor

Yup, agreed, thanks @clhenrick.

@bsmth
Copy link
Member

bsmth commented Feb 3, 2025

Hi all, what's the next steps for this PR? Is it clear what changes need to be applied to the prose? Do you want to come back to this one @clhenrick?

Thank you :)

@bsmth bsmth added the awaiting response Awaiting for author to address review/feedback label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Awaiting for author to address review/feedback Content:HTML Hypertext Markup Language docs size/xs [PR only] 0-5 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants