Improve accessibility of cover images #3546

xi · 2024-03-25T14:05:01Z

I recently did an accessibility review of a local vufind installation. I thought that some of the changes could be useful for the wider community.

However, I don't want to set up a full development environment. So someone definitely has to check if these work in the general case.

demiankatz

Thanks for sharing this, @xi -- I've left a few comments below. I defer to the expertise of others in areas of accessibility, but I'm leaving a few opinions and possible recollections of past related conversations. I'm hoping that @crhallberg and others can weigh in as well.

demiankatz · 2024-03-25T16:04:33Z

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

@@ -1,16 +1,15 @@
-<?php $alt = $this->link ? trim($this->driver->tryMethod('getTitle') ?? '') : ''; ?>
-<?php if ($this->link && !empty($alt)): ?><a href="<?=$this->escapeHtmlAttr($this->link)?>" class="record-cover-link"><?php endif; ?>
+<?php if ($this->link): ?><a href="<?=$this->escapeHtmlAttr($this->link)?>" class="record-cover-link" tabindex="-1"><?php endif; ?>


Is the intent here to get rid of alt text on cover images because they are essentially decorative and do not add useful information for a non-sighted user? If so, I think that is probably a reasonable change.

What is the purpose of the tabindex change here?

As far as I am aware, the cover appears next to the title, which is also a link. So both the alt text and the link are redundant. The link is still useful for mouse users, but for keyboard users it is better to skip it.

I don't think tabindex=-1 is appropriate here, since we want the link to be reachable by tab. It may be redundant in the search results, but not on the record view.

I wasn't aware that the same code is also used on the record view, so thanks for pointing that out.

Just to be sure: I was under the impression that this was a link to the record view. Does the record view link to itself here?

Would it be possible to distinguish the two cases? If not I am fine with dropping this change,

In search results, the thumbnail links to the record page. On the record page, the thumbnail links to a larger version of the image, when available. The thumbnail template is generic and receives the href it is supposed to link to from the calling code. We could add another parameter to the template if we need to differentiate behavior in a new way.

demiankatz · 2024-03-25T16:08:03Z

themes/bootstrap3/templates/search/results.phtml

@@ -121,10 +121,10 @@

 <?php /* Refine Search Options */ ?>
 <?php if ($recommendations): ?>
-  <div class="<?=$this->layoutClass('sidebar')?>" id="search-sidebar">
+  <aside class="<?=$this->layoutClass('sidebar')?>" id="search-sidebar">


I feel like the idea of turning the recommendation sidebar into an aside has been previously discussed and rejected, since the sidebar usually presents core functionality like faceting, which is more than (to quote the definition of aside) "only indirectly related to the document's main content." But maybe I'm misremembering. Maybe @crhallberg remembers this better than I do.

I think it is important to use some kind of landmark here. Note that some of these would also require an accessible label. Maybe region is a better choice here? form could also be a good candidate, but I fear that this would have too much of an impact on the contents.

region will be more appropriate with the right aria-label. I could have sworn there was a h2 denoting the search filters...

There is no h2 specifically above the region where recommendations are placed; instead, each recommendation block within that region has its own h2. In most configurations, the one in sidefacets.phtml ends up on top.

themes/bootstrap3/templates/layout/layout.phtml

crhallberg · 2024-03-25T18:51:01Z

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

@@ -1,16 +1,15 @@
-<?php $alt = $this->link ? trim($this->driver->tryMethod('getTitle') ?? '') : ''; ?>
-<?php if ($this->link && !empty($alt)): ?><a href="<?=$this->escapeHtmlAttr($this->link)?>" class="record-cover-link"><?php endif; ?>
+<?php if ($this->link): ?><a href="<?=$this->escapeHtmlAttr($this->link)?>" class="record-cover-link" tabindex="-1"><?php endif; ?>


I don't think tabindex=-1 is appropriate here, since we want the link to be reachable by tab. It may be redundant in the search results, but not on the record view.

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

themes/bootstrap3/templates/layout/layout.phtml

crhallberg · 2024-03-25T19:01:25Z

themes/bootstrap3/templates/search/results.phtml

@@ -121,10 +121,10 @@

 <?php /* Refine Search Options */ ?>
 <?php if ($recommendations): ?>
-  <div class="<?=$this->layoutClass('sidebar')?>" id="search-sidebar">
+  <aside class="<?=$this->layoutClass('sidebar')?>" id="search-sidebar">


region will be more appropriate with the right aria-label. I could have sworn there was a h2 denoting the search filters...

xi · 2024-03-25T21:46:43Z

A general question: Would you prefer me to push new commits on top of this or amend the existing commits and force-push?

demiankatz · 2024-03-25T22:36:07Z

A general question: Would you prefer me to push new commits on top of this or amend the existing commits and force-push?

Feel free to do whatever is easiest for you -- we generally "squash and merge" pull requests, so it's not necessary to keep a clean history here unless you want to keep separate changes in separate commits. (Keeping things separate is good for larger and more complex PRs, but I think this one is probably small enough to be understandable as a single commit).

themes/bootstrap3/templates/search/results.phtml

demiankatz

Thanks, @xi, this is looking very good. Just a few questions and comments below. If you're in a hurry to see some of these changes reflected in the dev branch right away, I don't mind cherry-picking the commits that fall outside of this review -- just let me know if that makes a difference to you or not.

module/VuFind/src/VuFind/View/Helper/Root/Record.php

demiankatz · 2024-04-02T17:34:15Z

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

 <?php /* Display thumbnail if appropriate: */ ?>
 <?php if ($cover): ?>
-  <img src="<?=$this->escapeHtmlAttr($cover); ?>" <?php if ($linkPreview): ?>data-linkpreview="true" <?php endif; ?>class="recordcover" alt="<?=$this->escapeHtmlAttr($alt); ?>">
+  <img src="<?=$this->escapeHtmlAttr($cover); ?>" <?php if ($linkPreview): ?>data-linkpreview="true" <?php endif; ?>class="recordcover" alt="<?=$this->transEscAttr('Cover Image') ?>">


Might it still make sense to have an empty alt in situations where the image is not linked and/or the link is not unique, to reduce noise, since the cover images are essentially just decoration in these situations?

Every link needs to have an accessible name, so if there is a link there also needs to be an alt-text. But I can adapt the code for the case if there is no link.

That's fair, but I wonder if the best thing to do would be to put aria-hidden="true" on the link for non-unique cover links. This way, our markup can be valid, but we can also hide useless repetitive "Cover Image" announcements from users who won't find them useful. What do you think?

My head is spinning a bit, so just to be sure:

if no link: empty alt

if non-unique link: non-empty alt and aria-hidden

if unique link: non-empty alt and no aria-hidden

That's what I implemented.

I think it is a little weird that the "no link" case is a little less hidden than the "non-unique link" case. An alternative approach could be to always have a non-empty alt and add a <div aria-hidden="true"> instead of the <a> for the "no link" case. What do you think?

Yes, if that makes the template simpler and more readable, that makes sense to me and should have the same effect.

One last question: since div is a block-level element and a is not, would it be better to use a span here for consistency of layout/flow?

That depends on the CSS. I assumed that since the a contains an img that there is some kind of special styling. In the EDS template it also contains <div class="ajaxcover">. But I don't know enough about the context to give a definitive answer. Do you think I should change it?

Note that a is no longer an inline element in HTML 5 https://stackoverflow.com/a/1828032.

I'll defer to @crhallberg on this question, but it sounds like I'm probably just behind the times in my knowledge of the HTML standards. Thanks for clarifying that!

themes/bootstrap3/templates/RecordDriver/EDS/cover.phtml

xi · 2024-04-03T08:06:48Z

If you're in a hurry to see some of these changes reflected in the dev branch right away, I don't mind cherry-picking the commits that fall outside of this review -- just let me know if that makes a difference to you or not.

Thanks for asking! I am in no hurry. I know that sometimes it can be useful to avoid merge conflicts to cherry pick some commits early while others are still in discussion. Feel free to do that if you think this would be beneficial. But there is no pressure from my side.

demiankatz

Thanks for the progress, @xi. I think we're nearly done here, but a couple more small thoughts:

themes/bootstrap3/templates/RecordDriver/EDS/cover.phtml

demiankatz · 2024-04-03T13:50:13Z

themes/bootstrap3/templates/RecordDriver/DefaultRecord/cover.phtml

 <?php /* Display thumbnail if appropriate: */ ?>
 <?php if ($cover): ?>
-  <img src="<?=$this->escapeHtmlAttr($cover); ?>" <?php if ($linkPreview): ?>data-linkpreview="true" <?php endif; ?>class="recordcover" alt="<?=$this->escapeHtmlAttr($alt); ?>">
+  <img src="<?=$this->escapeHtmlAttr($cover); ?>" <?php if ($linkPreview): ?>data-linkpreview="true" <?php endif; ?>class="recordcover" alt="<?=$this->transEscAttr('Cover Image') ?>">


That's fair, but I wonder if the best thing to do would be to put aria-hidden="true" on the link for non-unique cover links. This way, our markup can be valid, but we can also hide useless repetitive "Cover Image" announcements from users who won't find them useful. What do you think?

demiankatz

Thanks again, @xi -- this looks good to me now. However, I'm going to wait for more feedback from others before merging, just in case there are any considerations I am overlooking. Both @crhallberg and @EreMaijala are on vacation this week, but I hope to hear from one or both of them after they've returned and caught up.

EreMaijala · 2024-04-12T10:02:45Z

I think there may be one thing that complicates this: the record embedding support (see [List] section in searches.ini) in the results list. If enabled, the cover image is the only link to the full record page. Clicking the title just expands or collapses the record.

demiankatz · 2024-04-12T16:57:22Z

I think there may be one thing that complicates this: the record embedding support (see [List] section in searches.ini) in the results list. If enabled, the cover image is the only link to the full record page. Clicking the title just expands or collapses the record.

Good catch, @EreMaijala! I guess maybe we need to use $options->getListViewOption() to decide on the appropriate behavior in the search results. Maybe when tabs or accordions are enabled, we do need to go back to the behavior where the image alt text is the title of the book. May take some thought to figure out how to do this in the least convoluted way -- there are a lot of code paths to consider here.

demiankatz

Switching my review back to "request changes" until we can sort out the issue that @EreMaijala raised.

EreMaijala

Toggling my review status as well until the issue is resolved.

demiankatz · 2024-04-15T15:43:17Z

Thanks, @EreMaijala. Since this process is taking a while, I went ahead and cherry-picked the two uncontroversial commits out of this PR into dev and rebased. Might as well benefit from those pieces while we work through the challenges with this last part.

demiankatz · 2024-04-30T17:46:47Z

This hasn't moved in a little while, so I'm taking the 10.0 milestone off of it for the moment -- I'm not sure if it will get done in time for that release. I have also retitled the PR to reflect the parts that remain, since the other accessibility items were merged separately.

@xi, please let us know if you need more help from our end to continue moving this forward.

xi · 2024-05-01T09:32:32Z

Oh, sorry, it seems like there was a misunderstanding. I was actually waiting for you to tell me what to do. I know next to nothing about the issue that @EreMaijala pointed out, so I cannot decide how to deal with it.

demiankatz · 2024-05-01T13:12:22Z

@xi, I confess that this is a bit hard for me to wrap my head around as well, but maybe as a starting point we need to be a little more explicit about the behaviors. Maybe rather than using the $linkUnique argument to the getCover methods, we should add two different arguments:

$useTitleAsAltText = should we use the title of the item as the image alt text (true) or the generic "Cover Image" text (false)?

$ariaHidden = should we hide this element from screen readers and keyboard access (true) or not (false)?

The tricky part is figuring out how to set these arguments based on circumstances.

You can see the behavior that @EreMaijala mentions by setting "view" to either "tabs" or "accordion" in the [List] section of searches.ini. As noted earlier, it should be possible to use the Search Options object to figure out when it is turned on. Unfortunately, my memory of exactly how this is implemented is a bit hazy (I didn't write it, and it's been years since I reviewed it) and I'm short on time to refresh my memory... but that may be necessary if you need more help. Please let me know what you think! I probably won't have the capacity to do anything with this until after 10.0 is released due to the many tasks I have to take care of leading up to the release, but if it can wait that long, I don't mind helping further when I'm able... and maybe others can step up in the meantime. :-)

xi · 2024-05-13T16:04:11Z

Just to be sure: Why do we want to use the title as alt text when record embedding is used? @EreMaijala wrote:

If enabled, the cover image is the only link to the full record page. Clicking the title just expands or collapses the record.

For me that seems to imply linkUnique = true, but not that the text of the link must be the record title. Maybe "open full page" would be even better in that case.

demiankatz · 2024-05-13T18:25:21Z

Just to be sure: Why do we want to use the title as alt text when record embedding is used? @EreMaijala wrote:

If enabled, the cover image is the only link to the full record page. Clicking the title just expands or collapses the record.

For me that seems to imply linkUnique = true, but not that the text of the link must be the record title. Maybe "open full page" would be even better in that case.

I think the issue in this case is that every search result will have its own image link that opens the record page -- so there presumably needs to be a way to differentiate them from one another.

xi · 2024-05-28T16:14:20Z

Unfortunately I don't think I have time to work on this anymore. Thanks for all the constructive feedback! I hope that someone will be able to pick this up.

demiankatz · 2024-05-28T16:19:21Z

Thanks, @xi; I'm glad we were at least able to get all of your other improvements merged, and I'm sure we'll find time for a community discussion about how to finish this work at some point -- though likely not until after the 10.0 release is completed.

crhallberg · 2024-07-08T18:57:45Z

Quite late to this and I'm not sure if it simplifies it at this point, but if an image is the only child of a link, the image needs an alt attribute or the link needs an attribute. We can put an sr-only title inside the link allowing us to put alt="" on the image, since these are technically decorative.

https://russmaxdesign.github.io/accessible-forms/accessible-name-link03.html

demiankatz · 2024-07-08T19:54:11Z

@crhallberg, I think the complication here is that the thumbnails are usually decorative, but there are some edge cases where they are not -- for example, if you turn on record embedding (tabs/accordions), then the thumbnail is the only way to get to the actual record page. Record embedding is problematic for other reasons, but we need to either account for that problem in our solution here or else change the way embedding behaves.

EreMaijala · 2024-08-05T10:08:10Z

@demiankatz I think we need to change images to be purely decorative even with embedding. This of course means that we need "Show full record" link or something like that, which pollutes the display, but the current mechanism isn't obvious or easily discoverable. That'd get rid of this issue, and would e.g. make it possible to display a large image popup on click instead of going to the record page.

demiankatz · 2024-08-05T11:19:39Z

@demiankatz I think we need to change images to be purely decorative even with embedding. This of course means that we need "Show full record" link or something like that, which pollutes the display, but the current mechanism isn't obvious or easily discoverable. That'd get rid of this issue, and would e.g. make it possible to display a large image popup on click instead of going to the record page.

That seems like a reasonable solution, and we already have a "View Full Record" translation (currently used for linking to record URLs from results emails, so with a matching semantic meaning) that could be easily used for this. Maybe we should add that in a separate PR and then circle back to this one. Does anyone have time to do that? If not, I'll try to put it on my to-do list somewhere.

xi · 2024-10-01T19:48:10Z

I fixed the merge conflicts, so hopefully this can go into 10.1.

demiankatz

@xi, it looks like the templates are out of sync between the themes. You can fix this with: vendor/bin/phing update-bootstrap5. Do you mind doing that and committing the change? If you don't have time, I'll do it when I get home next week...

xi · 2024-10-02T20:50:59Z

it looks like the templates are out of sync between the themes. You can fix this with: vendor/bin/phing update-bootstrap5

It would be great if you could do that. I don't really have the relevant development setup available right now.

demiankatz · 2024-10-03T12:52:38Z

@xi, I had a few moments, so I just did it. I may not have time to fully re-test/re-review until I'm home next week, though. Stay tuned, and thanks for your help! :-)

demiankatz requested review from crhallberg and EreMaijala March 25, 2024 16:02

demiankatz requested changes Mar 25, 2024

View reviewed changes

crhallberg reviewed Mar 25, 2024

View reviewed changes

xi force-pushed the a11y-improvements branch from 59cc062 to 272ca70 Compare March 26, 2024 13:26

xi commented Mar 26, 2024

View reviewed changes

themes/bootstrap3/templates/search/results.phtml Outdated Show resolved Hide resolved

xi force-pushed the a11y-improvements branch from 272ca70 to 30523ac Compare March 27, 2024 15:14

xi requested a review from demiankatz April 2, 2024 09:35

demiankatz added the accessibility label Apr 2, 2024

demiankatz requested changes Apr 2, 2024

View reviewed changes

xi force-pushed the a11y-improvements branch from 30523ac to 45060e1 Compare April 3, 2024 08:04

demiankatz requested changes Apr 3, 2024

View reviewed changes

xi force-pushed the a11y-improvements branch from 45060e1 to 3a941c1 Compare April 4, 2024 16:43

demiankatz requested a review from crhallberg April 5, 2024 12:14

demiankatz added this to the 10.0 milestone Apr 8, 2024

demiankatz approved these changes Apr 8, 2024

View reviewed changes

demiankatz requested changes Apr 12, 2024

View reviewed changes

EreMaijala requested changes Apr 15, 2024

View reviewed changes

demiankatz force-pushed the a11y-improvements branch from 2dafd71 to 849c8a0 Compare April 15, 2024 15:42

demiankatz changed the title ~~Accessibility improvements~~ Improve accessibility of cover images Apr 30, 2024

demiankatz removed this from the 10.0 milestone Apr 30, 2024

xi force-pushed the a11y-improvements branch from 849c8a0 to 2d80409 Compare May 13, 2024 16:04

demiankatz added this to the 10.1 milestone Jun 20, 2024

demiankatz mentioned this pull request Aug 21, 2024

Add permalink to search results in embedded mode; adjust thumbnail links #3868

Merged

xi force-pushed the a11y-improvements branch from 2d80409 to 1e36dc4 Compare October 1, 2024 19:45

Improve accessibility of cover images

295c1e6

xi force-pushed the a11y-improvements branch from 1e36dc4 to 295c1e6 Compare October 1, 2024 19:48

demiankatz requested a review from EreMaijala October 2, 2024 17:25

Tweak whitespace.

610788c

demiankatz requested changes Oct 2, 2024

View reviewed changes

Update bootstrap5.

feca793

Improve accessibility of cover images #3546

Are you sure you want to change the base?

Improve accessibility of cover images #3546

Conversation

xi commented Mar 25, 2024 • edited Loading

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xi commented Mar 25, 2024

demiankatz commented Mar 25, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xi commented Apr 3, 2024

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

EreMaijala commented Apr 12, 2024

demiankatz commented Apr 12, 2024

demiankatz left a comment

Choose a reason for hiding this comment

EreMaijala left a comment

Choose a reason for hiding this comment

demiankatz commented Apr 15, 2024

demiankatz commented Apr 30, 2024

xi commented May 1, 2024

demiankatz commented May 1, 2024

xi commented May 13, 2024

demiankatz commented May 13, 2024

xi commented May 28, 2024

demiankatz commented May 28, 2024

crhallberg commented Jul 8, 2024

demiankatz commented Jul 8, 2024

EreMaijala commented Aug 5, 2024

demiankatz commented Aug 5, 2024

xi commented Oct 1, 2024

demiankatz left a comment

Choose a reason for hiding this comment

xi commented Oct 2, 2024

demiankatz commented Oct 3, 2024

xi commented Mar 25, 2024 •

edited

Loading