-
Notifications
You must be signed in to change notification settings - Fork 266
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
update to focus placement for modal dialogs #3214
base: main
Are you sure you want to change the base?
Conversation
techniques/failures/F85.html
Outdated
@@ -23,14 +24,14 @@ <h3>Setting focus to the document after dismissing a menu embedded on the page</ | |||
<p>Activate the trigger control via the keyboard.</p> | |||
<ul> | |||
<li>Check whether focus is in the menu or dialog.</li> | |||
<li>Check whether advancing the focus in the sequential navigation order puts focus in the menu or dialog.</li> | |||
<li>Check whether moving the focus forwards once in the sequential navigation order puts focus in the menu or dialog.</li> |
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.
Thinking of dialogs with no interactive content apart from the close icon. When focus is set here on opening, it is often not easy to see since there is no other focus position to go to - in effect you can either press enter or ESC to close. In these situations the second check would not work. Not sure if this edge case (not too infrequent though) needs to be covered...
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.
the "check" may not necessarily be a visual check. an auditor may also just check document.activeElement
.
techniques/failures/F85.html
Outdated
<ul> | ||
<li>Check whether focus is on the trigger control.</li> | ||
<li>Check whether advancing the focus backwards in the sequential navigation order puts focus in the trigger control.</li> | ||
<li>Check whether moving the focus backwards once in the sequential navigation order puts focus in the trigger control.</li> |
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.
"focus in" > "focus on" ?
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.
Can't this be said in less technical language?
Noting the latest comment on the thread, it seems we need another clause for each as otherwise the failure is stricter than the normative text. |
Revisiting this, there needs to be more updates to the document to spruce up the commentary on DHTML elements. There's also a couple of comments in the PR that need addressing. I'll take a crack at it. |
a slight concern overall that the "see if you tab forward or back gets you where you'd expect" test may miss situations where browser/AT error-correct for lost/reset focus (when |
✅ Deploy Preview for wcag2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
have assigned to you @fstrr and moved back to in progress |
@patrickhlauke In your most recent comment, are you suggesting that the "see if you can tab forward/backwards" test should come out? |
1. Remove content about pressing the Tab key to find the triggering control. 2. Add a check to allow focus to be placed on a logical other control if the triggering element has been removed from the DOM. 3. Updated the “DHTML” content.
Discussed on TF call 11/22. @fstrr please add bullet points or summary of what's changed, since |
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.
some suggestions to consider. some of these might go a bit beyond what was originally meant to change in this PR, but I think add more clarity to the updates made.
otherwise, i'm good with this update.
Co-authored-by: Scott O'Hara <[email protected]>
Co-authored-by: Scott O'Hara <[email protected]>
Co-authored-by: Scott O'Hara <[email protected]>
Co-authored-by: Scott O'Hara <[email protected]>
Summary of changes:
|
Discussed on call 12/6, see preview diff. |
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 for pulling in so many of my suggestions. i'm good with how this ended up
Changed "the next" to just "next". Might also be "the next item"
Closes #518 by adding some more details around focus handling, both inside dialogs and where the triggers for a dialog no longer exist with the dialog closes.
See a preview diff