Add media type context to start of result link titles #1920
Conversation
sarayourfriend
added
🟧 priority: high
| ? // TODO: Confirm with @dhruvk when this _shouldn't_ be the "seekable" variant | ||
| // `seekable` variant for the string wasn't used before and changing this makes | ||
| // non-seekable variant unused: can we remove one or the other and just use one? | ||
| i18n.t("audio-track.aria-label-interactive-seekable", { |
There was a problem hiding this comment.
@dhruvkb Can you confirm what the correct label should be here? I haven't gone digging for when these labels were introduced or if the seekable one used to be used but was accidentally removed at some point. We don't use both, so we can remove the one we don't use if this change doesn't require additional work to choose the correct one.
In what cases is the player interactive but not seekable? At first I thought it might be the box layout (used for all results grid) but that one is seekable when I tested 🤔
There was a problem hiding this comment.
I recall the box layout not being seekable previously. It isn't via mouse, due to the link, therefore I'm not sure if it should be by keyboard 🤔
There was a problem hiding this comment.
@dhruvkb pinging you here again for visibility, thank you! I don't believe the box layout should be seekable.
There was a problem hiding this comment.
Yes, the box layout should not be seekable. It was considered bad UX to allow it to be seekable when the seeking area is super tiny and lies inside a link.
There was a problem hiding this comment.
Got it, thanks! So currently the box layout is seekable, so as-written this code will work fine. I'll remove the comment and create a new issue to make the box layout not-seekable, at which point the labels should be updated.
|
Size Change: +368 B (0%) Total Size: 841 kB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/1920 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
|
After much debugging and fighting with Microsoft's servers to actually successfully download the Playwright Docker image from their registry and then the browser binary from their distribution endpoint, both of which were impossibly slow on my connection, I finally found the issue. The audio cells did not have I tried to add Instead of trying to continue fighting with it, I opted for the navigation code to fall back to aria-label if title is empty. This avoids the issue entirely and still exercises the context that a screen-reader user would have for the link. |
|
I wrote a long comment about I think the audio problem with the title might be caused by the fact that |
| @@ -130,12 +132,19 @@ export default defineComponent({ | |||
| imgWidth.value = element.naturalWidth | |||
| } | |||
|
|
|||
| const contextSensitiveTitle = computed(() => { | |||
| return i18n.t("browse-page.aria.image-title", { | |||
| title: props.image.title, | |||
There was a problem hiding this comment.
Do you think it would be clearer for screen reader users if we added more info, something like the full attribution sentence (Image: Cat by creator licensed under CC BY licensed, maybe even adding the provider)?
There was a problem hiding this comment.
I support this idea. Perhaps it's better-suited to a follow-up PR though, as I'd want to make sure, and check with a11y folks, that the titles wouldn't be too long and hinder the search browsing experience.
There was a problem hiding this comment.
This is an improvement for the Image results, so I'm approving to unblock it, but I think we need to make the experience for audio and image results more unified.
For images screen reader reads "Link Image Cat", and for audio it reads "Cat audio player, press spacebar to play" - and has no indication that the element is actually a link. I don't know what the best solution here is, though: would adding something like "Press enter to go to the Audio details" to the end of the audio player instructions be enough?
Also, I asked a question in an inline comment: would it improve the screen reader user experience if we add more information about the images? Because currently, they don't get much info about the image beyond its title and media type.
Thanks for testing this. I haven't had access to a screen reader and need to fix that soon; realistically I probably shouldn't have taken on this issue due to that. I agree audio should read similarly to images. I don't know if our current approach might be annoying though: it's quite wordy. For a page full of audio results, moving through them would be pretty chatty already, with the instructions repeated on each one. Could we implement something like the Gutenberg's I'm also not sure how best to approach this but do agree that it is suboptimal. This issue was stemmed from #448 which has a host of other already-identified improvements. My inclination is that we should prioritise fixing those issues, especially because the issue describes specific fixes for almost everything listed. |
I agree with prioritizing those issues. I think this PR adds a good incremental change for the Image results, and could be merged because it's useful as is. We should also open an issue for improving the overall experience for the search results, and exploring the a11y utility you mentioned. |
obulat
force-pushed
the
add/sr-link-context-search-views
branch
from
4ce8223 to
0168b22
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was ready for review 10 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
Co-authored-by: Olga Bulat <obulat@gmail.com>
) * Revert PW changes from #1920 * Try removing only * Try removing only * Fix url path matching in "should allow toggling audio playback via play/pause click" test --------- Co-authored-by: Zack Krida <zackkrida@pm.me>
Fixes
Fixes #1010 by @sarayourfriend
Description
Adds SR accessible context as per the issue. There are two "todos" that I'll ping folks on to clarify what to do with them.
I also made some changes to the playwright configuration to make it easier to use
startinstead ofprodto quickly re-run the tests inside the container. I can revert these changes if they're not wanted, but it is merely a codification of what was present in the comment I moved and updated.Testing Instructions
Confirm CI runs. I changed a test to exercise the accessible title.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin