Image: Improve focus management in lightbox #55428
Merged
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
This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes.
|
Size Change: +1 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
|
One issue with setting the focus on the dialog itself as recommended in #55414 is that this causes unexpected behavior in Safari. Usually it will cause VoiceOver to say that focus is on the close button, but it will not respond to an I've recorded a video showing this behavior while casting the keystrokes to the screen: Lightbox.Focus.Test.mp4 |
|
Flaky tests detected in 6c1090a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6578277725
|
|
I'm ready to get additional eyes on this and suggestions on how to improve the implementation. As of this writing, I've been unable to solve this issue in Safari while setting focus on the modal itself, i.e the element with |
artemiomorales
added
[Type] Bug
It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox.
|
@afercia After looking at this with @SantosGuillamot, I don't think there's a way to set focus to the whole modal dialog without causing unexpected behavior (see video above). So with that in mind, I've set the focus back to the close button, and I've also added a I know you're not a fan of making the image itself clickable, but are there any technical drawbacks to adding a |
artemiomorales
requested review from
mtias,
jameskoster,
jasmussen and
richtabor
@artemiomorales thanks for looking into this.
This is expected behavior. There is one thing that behaves differently compared to the editor Mocal component though. As an aside and out of curiosity, I spent some time trying to debug the Safari + VoiceOver behavior. I think it's something specific to Safari and the aria-modal attribute.
So it appears that Safari, when it sees an aria-modal attribute, now moves focus to the first focusable element within the element with aria-modal. To me, this is a bug. It may be related to recent work that browser vendors have been doing to align their implementations with the new focus steps for the HTML |
|
@aristath @carolinan and myself had a look at this and we think it's the smallest amount of changes we can introduce since we are in RC1. We recommend final review and testing with all major browsers. @joedolson @alexstine I'd appreciate your thoughts and testing, especially with Windows screen readers. Thanks. There are outstanding, broader, issues that would require some discussion and this is not the right time for that, as we are in RC1:
We agree all these points can't be addressed now. The should be addressed soon in the next release though. |
afercia
reviewed
Oct 19, 2023
alexstine
approved these changes
Oct 19, 2023
jameskoster
approved these changes
Oct 19, 2023
michalczaplinski
approved these changes
Oct 19, 2023
@afercia Thank you for your detailed analysis and exploration. The approach we have now looks good to me. I've pushed a change removing the extra |
|
Thanks everyone for the PR, review, and feedback. From an a11y perspective, this looks good to me though I'd like to see it tested with WIndows screen readers. We can investigate the different behavior of Safari / VoiceOver with the Lightbox modal dialog compared to the editor Modal component later in teh next release cycle. |
|
I quickly tested on gutenberg.run with latest Firefox and NVDA.
Notice the doubled images within the dialog are both announced. I will create a follow-up issue to evaluate improvements for the pending points. |
alexstine
approved these changes
Oct 20, 2023
There was a problem hiding this comment.
@artemiomorales Tested on Windows with NVDA in Google Chrome and Firefox. This works well enough for now. Let's land it.
|
Great, thank you all for the feedback and testing! This has been merged and will follow up on outstanding items in the next release cycle. |
SiobhyB
pushed a commit
that referenced
this pull request
Oct 20, 2023
* Improve focus management This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes. * Add delay before focusing when closing lightbox * Put focus back on close button when opening lightbox It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox. * Move focus to the dialog when opening the lightbox. * Fix SVG markup. * Consistent indentation with spaces. * Remove unnecessary tabindex --------- Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
|
Cherry-picked for inclusion in 6.4 in feefbe1. |
SiobhyB
removed
the
Backport to WP 6.7 Beta/RC
SiobhyB
pushed a commit
that referenced
this pull request
Oct 23, 2023
* Focus submenu button when clicked (#55198) * Focus element manually when open submenu on click * Try using `tabindex="-1"` * Use `tabindex="-1"` also in body when a submenu is opened * Replace tabindex with event listener * Explain the tabindex on <li> * Don't store the element on hover to restore the focus later * Improve explanations * Add tests to cover webkit frontend menu interactions Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari. * Focus clicked button on Safari Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu. * Added the document.addEventListener body click back in Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change. * Remove tab keypresses from webkit menu interaction tests Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress. * Use body click instead for consistency across environments --------- Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Make layout support compatible with enhanced pagination (#55416) * Make layout supports compatible with enhanced pagination * Use sanitize_title and add `layout` to the class name * Update default fullscreen icon for lightbox trigger (#55463) * Make duotone compatible with enhanced pagination (#55415) * Patterns: fix capabilities settings for pattern categories (#55379) Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Revert "Patterns: fix capabilities settings for pattern categories (#55379)" This reverts commit 6f83c92. * Image block: wrap images with hrefs in an A tag (#55470) * This commit sees what happens when we wrap the image element in an A tag in the editor. This is to ensure any inherited styles from the link element, such as border color, apply to the image. * Removing duplicate <a href /> wrapper Adding disabled onClick and aria attribute * Image: Improve focus management in lightbox (#55428) * Improve focus management This commit removes the logic to set focus differently based on event.pointerType and instead sets focus on the dialog itself when the lightbox opens, and on the lightbox trigger when the lightbox closes. * Add delay before focusing when closing lightbox * Put focus back on close button when opening lightbox It turns out that placing focus on the modal was causing inconsistent behavior in Safari, so I've put the focus back on the close button when the lightbox opens, which performs predictably. I've also added a tabindex to the image, which prevents the focus ring from erroneously showing when opening the lightbox with a mouse in Chrome and Firefox. * Move focus to the dialog when opening the lightbox. * Fix SVG markup. * Consistent indentation with spaces. * Remove unnecessary tabindex --------- Co-authored-by: Andrea Fercia <a.fercia@gmail.com> * Fix: - Update the title when using enhanced pagination --------- Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com> Co-authored-by: Luis Herranz <luisherranz@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Artemio Morales <artemio.morales@a8c.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
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.
What?
This PR makes the focus management more consistent in the image lightbox by removing the reference to
event.pointerTypeand the differentiation in behavior when using a mouse versus a keyboard.Why?
Addresses #55414 and improves on changes made in #55212.
Altogether, the previous approach was unreliable, so this PR aims to fix that.
How?
It removes the logic using
event.pointerType, and instead...tabindexof -1 to the image, which prevents the focus ring from erroneously showing around the close button when the lightbox is opened in Chrome and Firefox (the focus ring appears to always be visible in this case in desktop Safari, which appears to be the intended browser behavior).Testing Instructions
On keyboard, test that tabbing to the image will show the lightbox trigger button on focus and that pressing
EnterorSpacebaropens the lightbox as expected.While the lightbox is open, test that...
Escapecloses the lightboxTabwill move focus to the close button and trap itEnterorSpacebarwhile on the close button will close the lightboxWhen the lightbox closes, make sure focus is set on the lightbox trigger.