Add alt attribute to image on PCPInfo template. #21808
Conversation
|
(Standard links)
|
|
@agh1 this feels like something you know best practices for |
CRM/PCP/Page/PCPInfo.php
Outdated
| @@ -185,10 +185,11 @@ public function run() { | |||
| if (!empty($entityFile)) { | |||
| $fileInfo = reset($entityFile); | |||
| $fileId = $fileInfo['fileID']; | |||
| $altText = $fileInfo['description'] ?? ''; | |||
There was a problem hiding this comment.
Because 'description' is not automatically html-escaped (per https://github.com/civicrm/civicrm-core/blob/5.43/CRM/Utils/API/HTMLInputCoder.php#L55) it needs to be escaped here before inserting it into html.
There was a problem hiding this comment.
Good catch. I've updated this to use htmlspecialchars, but to be honest I wasn't sure if CiviCRM has a better function to be using here - let me know if so!
|
@braders thanks for this PR, this looks good to go, but it would be helpful if you squashed the 2 commits (which you can do with |
It is an accessibility best-practice that all images should have alt text. If a description has been provided, we use that text, otherwise an empty alt text is used (i.e. the image is treated as decorative)
braders
force-pushed
the
feature/pcp-alt-text
branch
from
8691f56 to
7546e57
Compare
|
@colemanw Rebased. Thanks |
|
Whoa y'all I'm sorry I'm late to the party, but I don't think it is clear to the person editing the description that it's to be used as alt text. Yes, alt text is better than no alt text, but no text is better than random text as alt text. I think it will be important to modify the PCP edit form to specify that it's not "description" but rather alt text. We'll also need to make it clear at least in the release notes, if not in the upgrade messages, that all past PCP image descriptions are now treated as alt text. On the plus side, my general sense is that few people have been using the description field and if they do, they're unlikely to put anything too terrible in there. It just might be a little confusing for someone who can't see the image. |
|
For what it's worth, I'm not opposed to the alt attribute always being blank (i.e. treating the image as decorative). Given that the description is otherwise not displayed on the PCP page, using it as alt text feels appropriate, but I don't feel strongly either way. |
|
I actually do think this is an improvement--it just needs to be followed back to the source a little more. |
It is an accessibility best-practice that all images should have alt text.
If a description has been provided, we use that text,
otherwise an empty alt text is used (i.e. the image is treated as decorative)