-
Notifications
You must be signed in to change notification settings - Fork 4.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
Image: fix focus management and accessibility of the Lightbox #55414
Comments
@afercia Thanks for opening this issue! I've opened a PR to address the focus management items. Here follow some notes:
As you've suggested before, the solution here will probably require implementing tooltips or something similar and will require a larger effort to get right.
The answer here is likely to allow users to configure the size of the button. For now, I'd prefer to start with a small button. Perhaps @richtabor would like to weigh in?
I'm on board with this, but after implementing it, I've run into a problem with Safari when using VoiceOver wherein focus is announced to be on the Close button but using the
This discussion was brought up in a previous issue as well — I'll refer to this comment, which captures my thoughts. To summarize, I think having a clickable image and a visual hint on hover is a good balance of offering additional functionality while also preserving the core UX of offering a good reading experience, though am happy to hear other ideas. |
Thanks @artemiomorales. The Yoast team will have a look at the PR today during a mob-coding session.
I agree this can't be addressed now as it's a broader issue. However, I would have liked to see a setting to change the appearance of the buttons from icon to text as done for the Navigation, also for consistency. I'm not sure I understand why users can control the buttons appearance of the Navigation and they can't do for the Lightbox: I already planned to create a separate issue for this.
We should make sure that any markup output by WordPress is accessible and compliant. Allowing users to configure the size to enlarge it is certainly a good thing. But, we should force a minimum size of 24 by 24 pixels. |
Regarding the visual hint, I kindly disagree. This: doesn't let users understand the image can be enlarged. Visually, there's no way to understand it, unless that by accident useers hover or focus the image. I'm not sure this is any good. I'd also liek to remind everyone that on mobile there is no hover. |
Description
Follow-up to #55212
#55212 improved the Lightbox implementation by changing the button that overlid the entire image to a smaller button placed at the top right of the image. That's a welcome change that solves a couple important issues. It also introduced a different implementation of focus handling of the modal dialog that needs to be fiexed.
Please refer the inline review on #55212
Quoting from the last comment there:
event.pointerType
is buggy, as Chrome and Opera return an empty string (as expected) when using the keyboard while Firefox and Safari returnundefined
.Depending on the flow used to open the dialog and on the browser in use, this is what I observed when testing:
Step-by-step reproduction instructions
Testing on macOS:
Screenshots, screen recording, code snippet
No response
Environment info
No response
Please confirm that you have searched existing issues in the repo.
Yes
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Yes
The text was updated successfully, but these errors were encountered: