-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Wrapped anchor tag around the image and caption for gallery block #14888
Conversation
I just wanted to repeat the question from @mapk:
If we get the green light for this approach, we will have to add an entry to Question posted in #accessibility channel on WordPress Slack (link requires registration): https://wordpress.slack.com/archives/C02RP4X03/p1554878707014200 |
@ashwin-pc, could you add an example of the generated markup in the issue? It would make it easier to review this PR. |
Stepping in after @gziolo invitation to comment on the proposed new markup. See also general considerations on the related issue #14304 (comment) While the link wrapping the figure is valid markup, from an accessibility perspective things are pretty different because the semantics changes. With the new markup, the gallery is actually a list of links instead of a list of figure elements. Considering semantics in an abstract way, we could debate if the caption being part of the link is valid markup. To me, it is not because the linked "object" should be the image. However, I'm more interested in the practical consequences of the new markup in the way the content is perceived and announced by assistive technologies. The experience of users who use assistive technologies is way more important than academic discussions about semantics. Testing with a couple browser / screen reader combinations, turns out there are relevant differences in the two markups. In the screenshot below: Firefox and NVDA before and after:With the current markup, a linked image is announced in a pretty clean way:
With the new markup:
Safari and VoiceOverBefore: Every piece of relevant information from the content is announced: After:
Note: VoiceOver offers the ability to "Interact with an item" via the dedicated shortcut RecapInconsistencies between browsers / screen readers behaviors are pretty normal when they encounter some unusual, unexpected, markup. Regardless, it appears the new markup produces a pejorative experience for assistive technologies users and I'm afraid I can't recommend this change. I'd suggest to explore an alternative approach. I'd tend to think the root cause of this issue is the CSS positioning, therefore this would probably better solved with CSS. Some thoughts:
I'd also like to hear thoughts from other accessibility team members, will take care of soliciting feedback. |
We shouldn't be changing the semantics (for everyone) to achieve a bigger hit area (for some users). Instead we should be enhancing the experience using JS. In this case we can take the destination of the link and bind a click event on its relating image. This is a plugin that does that for demonstrative purposes. We do not have to use that plugin, but I am showing the implementation that I recommend we achieve: http://lawlesscreation.github.io/jquery.clickable/ |
@afercia I'd like to propose another solution which i believe will not change the semantics for a screen reader, you can correct me if I'm wrong on this. Instead of wrapping the figure in the anchor tag, we can wrap another anchor tag around the caption too but set Before (Original): <figure>
<a href="#">
<img src="" alt="">
</a>
<figcaption>Caption</figcaption>
</figure> After: <figure>
<a href="#">
<img src="" alt="">
</a>
<a href="" aria-hidden="true">
<figcaption>Caption</figcaption>
</a>
</figure> This change will address both the users ability to select the caption and have a bigger hit area while not affecting accessibility. |
@ashwin-pc unfortunately |
Discussed during today's accessibility bug-scrub. Removing the "Needs Accessibility Feedback" label, as initial feedback has been given. Keeping the "Accessibility" label will still allow anyone interested in a11y to participate. |
I think @afercia provided some reasonable alternatives that meet a11y requirements.
Can we try this? Hide the caption on hover if the caption covers the image.
I suggest we keep the caption as selectable.
I'd like to see us offer different styling alternatives for captions as mentioned in the issue here. This could totally be another PR though. |
@ashwin-pc, can you just hide the caption on hover of the image for now? Let us know if you would like to continue working on this fix. I can take it over if you are busy with something else now that a few weeks have passed. |
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.
See my previous comment.
@gziolo Sure, you could take this over. I'm slightly tied up at the moment and not entirely sure what's expected of the fix. |
Worth considering one of the previous questions
Hiding the caption when hovering it would make impossible to select and copy the caption text. |
This is correct. I'm willing to take this tradeoff because I believe clicking an image to view it at a larger scale is the prefered action here. This PR can work toward this, and in the future we can create a PR that adds another style variation where the captions are below the images. |
Thanks for letting us know. I will close this PR. Thank you for your contribution, it helped to clarify what needs to be done 👍 |
Description
Fixes #14304. In the saved version of the post the anchor tag is now wrapped around the whole figure and not just the image. This is done only in the saved post to maintain interaction with the caption editor in the edit mode.
How has this been tested?
npm test
Link To
property being set. WhenLink To
is notNone
the entire image can be clicked. When it is set toNone
, clicking the image does nothing.Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: