-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: accessibility issue modal #2076
fix: accessibility issue modal #2076
Conversation
Can you remove the |
@@ -97,6 +97,11 @@ const Modal = ({ | |||
if (hideFooter) { | |||
classes += ' modal-footer-none'; | |||
} | |||
|
|||
useEffect(() => { | |||
document.getElementsByTagName('dialog')[0].showModal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think useRef
should be used here instead of accessing the DOM directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -97,6 +97,11 @@ const Modal = ({ | |||
if (hideFooter) { | |||
classes += ' modal-footer-none'; | |||
} | |||
|
|||
useEffect(() => { | |||
document.getElementsByTagName('dialog')[0].showModal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid document.getElementsByTagName
it's generally not a good practice to access dom directly in react.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here d467d83
I tried here but it didn't get updated 😬 |
e951e78
to
d467d83
Compare
The file is changed in 2 commits this one will be a bit tricky ! 1 ) Checkout the file from the main branch git checkout main -- package-lock.json
git commit -a -m "revert package-lock.json"
git rebase -i HEAD~4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shrilakshmishastry as far as understand the issue (from description and screen recordings) is that focus is going out of the active modal/dialog when navigating through the keyboard and hence you are trying to make sure that the focus stays in active dialog. Please let me know if my understanding is correct.
However the solution of triggering showModal
when mounted isn't clear to me and how is it fixing this issue ?
We should instead have a keydown handler for modal component which makes sure that on keyboard navigation the focus always keeps looping (in order) inside the active dialog unless esc is pressed ?
A small suggestion regarding the screen recording - you can directly upload the .mov
files so reviewers don't have to download the files to view the screen-recordings :)
For reference - I came across this PR in https://twitter.com/shrilakshmihg/status/1779420061721035153
Hey @ad1992 thanks for the suggestion!! yes, the focus is going out of the modal, hence the fix. calling The intention of using the you can read more about it here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog The doc says you need to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested a minor change otherwise looks good @shrilakshmishastry
I am not sure if there is a way to test the changes directly in the browser so I haven't tested it however the code looks good!
Additionally, I think you probably might be targeting the rest of the clean-up in separate PR (eg closing the modal via dialog close and removing ESC handler etc)
import React, { useEffect, useState } from 'react'; | ||
import StyledWrapper from './StyledWrapper'; | ||
import { useRef } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import React, { useEffect, useState } from 'react'; | |
import StyledWrapper from './StyledWrapper'; | |
import { useRef } from 'react'; | |
import React, { useEffect, useState, useRef } from 'react'; | |
import StyledWrapper from './StyledWrapper'; |
Since there are react imports already so better to club with it
closing modal is taken care in |
@shrilakshmishastry as per the docs, I think you can remove the |
We need it, as there are cases where we needn't close the modal on the click of |
@@ -1,6 +1,6 @@ | |||
import styled from 'styled-components'; | |||
|
|||
const Wrapper = styled.div` | |||
const Wrapper = styled.dialog` | |||
color: ${(props) => props.theme.text}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add:
max-height: unset;
max-width: unset;
To remove the weird gap around the backdrop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please elaborate more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dialogs now have a "weird" gap between the window border and the backdrop:
This is because the dialog
Element has some weird default CSS. From the chrome source:
dialog:-internal-dialog-in-top-layer {
/* https://html.spec.whatwg.org/multipage/rendering.html#flow-content-3 */
position: fixed;
overflow: auto;
inset-block-start: 0;
inset-block-end: 0;
max-width: calc(100% - 6px - 2em);
max-height: calc(100% - 6px - 2em);
/* https://github.com/w3c/csswg-drafts/issues/6939#issuecomment-1016679588 */
user-select: text;
visibility: visible;
}
Unsetting the default max-width
and max-height
will remove the gap.
color: ${(props) => props.theme.text}; | |
color: ${(props) => props.theme.text}; | |
max-height: unset; | |
max-width: unset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks !!
fixed it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Its-treason can you check now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the "toasts" are now in the background, behind the dialog/backdrop instead of the foreground. This is probably due to how the dialog
element works: https://stackoverflow.com/a/41816764
Seems intersting, I will look into this. Thanks |
The toast needs to be attached to or decendent of |
@anusreesubash Please review this? |
Description
Accessibility issue in Modal
Ally pattern of modal:
Modal is a window overlaid on either the primary window or another dialog window. Windows under a modal dialog is inert.
That is, users cannot interact with content outside an active dialog window.
Inert content outside an active dialog is typically visually obscured or dimmed so it is difficult to discern, and in some implementations, attempts to interact with the inert content cause the dialog to close.read more
Issue:
When the modal is open windows under a modal dialog are accessible by the keyboard.
Link to issue description
Fix:
Use
dialog
tag and callshowModal()
method on the mount of Modal componentScreen recording
Contribution Checklist: