Fix Patterns modal dialog focusable buttons and focusOutside for Safari. #56162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
|
Size Change: +30 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Nov 16, 2023
| @@ -777,3 +777,9 @@ $block-inserter-tabs-height: 44px; | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| :focus { | |||
There was a problem hiding this comment.
I believe it's just for debugging and not meant for merging 😅?
Edit: I saw the PR description, but let's mark it here in case anyone missing it and merge it by accident.
There was a problem hiding this comment.
Yes it is. I switched the PR to draft, just in case.
|
If this proves to be the right approach, it will also prove that a modal dialog can not contain focusable elements that are dynamically made non-focusable. |
There was a problem hiding this comment.
Thanks for the fix here @afercia 👍
I can confirm this PR fixes the reported issue in Safari. I might defer to others though regarding the approach.
✅ Patterns explorer modal no longer closes when selecting a category
✅ Tabbing is constrained to the pattern explorer modal now
✅ Tests well across Safari, Chrome, Firefox, and Edge
I did encounter one further issue in Safari when I went to test keyboard navigation. It appears that you cannot navigate to the inserter tabs. This worked fine in Chrome, FF, and Edge.
| Safari | Chrome |
|---|---|
Screen.Recording.2023-11-17.at.4.26.14.pm.mp4 |
Screen.Recording.2023-11-17.at.4.27.29.pm.mp4 |
The above issue might be out of scope for this PR but it could be a good follow-up. What do you think?
|
@aaronrobertshaw thanks,
I cannot reproduce. Works as expected for me with Safari 17.1.
Did you check your macOS settings and Safari settings?
|
I'd agree the approach would need some discussion. The main point here is that there's the need to make sure any content within the modal dialog dosn't trigger a focus loss. In this case a focus loss occurred for two different reasons:
I'm not sure documentation and developers education would be enough to make sure this doesn't happen again. Does anyone can think of a good way to make sure there are no focus losses?
|
Thanks, @afercia, I'd missed checking those settings. After correcting them everything works as expected. Sorry for the false alarm 🙏
I don't know if there is a guaranteed way to ensure this. Based on whether it is a child of a Modal, the conditional requirement of the Button prop makes me think we can't rely on enforcing this through typing. A linting rule might help but again I'm not sure we can create something that 100% flags the issue for developers. |
What about considering to throw a 'doing it wrong' or similar for the In most of the cases, the
See W3C: ARIA Authoring Practices Guide (APG) > Developing a Keyboard Interface > Focusability of disabled controls |
My vote would be to move the more global prevention of this problem to a dedicated issue and separate follow-up so as to not block the fix in this PR. We can then take the time to have a broader discussion and seek more feedback. What do you think? |
afercia
force-pushed
the
try/patterns-modal-dialog-focus-fixes
branch
from
eb97139
to
1a09fb0
Compare
kevin940726
reviewed
Nov 21, 2023
| @@ -22,6 +22,7 @@ function PatternCategoriesList( { | |||
| onClick={ () => { | |||
| onClickCategory( name ); | |||
| } } | |||
| tabIndex={ 0 } | |||
There was a problem hiding this comment.
Question: Should we maybe just add tabIndex={0} to the <Button> component to hide this workaround in an abstraction? IIUC, this doesn't impact other browsers and is actually the spec-compliant. Does adding this introduce any side-effect? Obviously we can do this in a separate PR too.
There was a problem hiding this comment.
@kevin940726 that is certainly something to consider to solve the Safari bug. I'm a little thorn about fixing specific browser bugs this way but yes we need to do something. As you said, a tabindex="0" attribute on buttons is valid, although redundant. We could abstract it, we just need to take care of the cases where a Button component has tabindex="-1" that may be changed dynamicall, for example in some ARIA patterns. I'd agree it should be addressed in a separate issue and PR.
I'm a little hesitant with introducing an ad-hoc fix only for this modal dialog. However, I don't have a strong preference. I'd be fine with merging this as long as we create a separate follow-up issue / PR for a broader discussion and feedback. For now, I will add some inline comments and remove the Draft status from this PR. |
afercia
force-pushed
the
try/patterns-modal-dialog-focus-fixes
branch
from
0d8a8e0
to
3ce5c15
Compare
gziolo
added
the
[Status] Stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #55110
This is only a proof of concept for now, please do not merge.
I'm using a thick red outline styling for focus, to make testing easier. This styling should be eventually removed of course.
What?
The patterns categories modal dialog is buggy with Safari and it closes unexpectedly.
Also, using the pagination buttons makes constraining tabbing within the modal fail.
Why?
Safari does not set focus on buttons when clicking them, this needs a workaround to prevent the modal dialog from closing unexpectedly.
Pagination buttons get disabled dynamically, this needs to be avoided to make constrained tabbing work.
How?
-Uuses aria-disabled in favor of disabled
The purpose ot this PR is to prove that interactive controls within the content of the modal dialog must always be focusable.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast