Skip to content
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

VUFIND-1673 Set unique labels for checkboxes #3552

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

maccabeelevine
Copy link
Member

Per the accessibility report from Moravian, the checkboxes in various lists need to have unique labels. Moravian suggests using aria-labelledby and linking to the ID of the title's anchor element, but that link often does not have an ID, so it would have to be added. Seems easier to just spit out the title directly into the label.

See https://openlibraryfoundation.atlassian.net/browse/VUFIND-1673

@maccabeelevine
Copy link
Member Author

There are various other checkbox lists that perhaps should follow the same approach, but I think feedback is helpful first.

select_item = "Select item for further action"
select_item_checked_out_renew = "Select item for renewing"
select_item_hold_cancel = "Select item for canceling holds or recalls"
select_item_ill_request_cancel = "Select item for canceling interlibrary loan requests"
select_item_purge = "Select item for purging"
select_item_storage_retrieval_request_cancel = "Select item for canceling storage retrieval requests"

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below for one technical question.

Beyond that, the bigger question is whether it's okay to replace an action-oriented label with just a title. Do we need to combine the two, e.g. "Select %%title%% for renewal"? I realize that making that change would require a significant amount of retranslation work, so it's not ideal... but losing the action label entirely also seems less than ideal.

Let me also draw your attention to #2999, which I believe is work in progress on code that's meant to be a foundation for solving this problem... but I think it's currently a bit stalled and needs more work.

@@ -1,5 +1,6 @@
<label class="record-checkbox hidden-print">
<input class="checkbox-select-item" type="checkbox" name="ids[]" value="<?=$this->escapeHtmlAttr($this->id) ?>"<?php if (isset($this->formAttr)): ?> form="<?=$this->formAttr ?>"<?php endif; ?> aria-label="<?=$this->transEscAttr('select_item')?>">
<?php $title = $this->titleHtml ?? $this->escapeHtmlAttr($driver?->getTitle() ?? $this->id); ?>
<input class="checkbox-select-item" type="checkbox" name="ids[]" value="<?=$this->escapeHtmlAttr($this->id) ?>"<?php if (isset($this->formAttr)): ?> form="<?=$this->formAttr ?>"<?php endif; ?> aria-label="<?=$title ?>">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay for aria-label to include HTML? I think by definition, it's possible that $this->titleHtml will contain markup (e.g. highlighting-related <span>s). While I think it's unlikely that would happen in this particular context, maybe it's worth running a strip_tags on the output just to be on the safe side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@demiankatz demiankatz added this to the 10.0 milestone Apr 2, 2024
@demiankatz
Copy link
Member

@maccabeelevine, I'm concerned that we may be losing as much as we gain here -- the old aria-label attributes explained what the checkboxes do, whereas the new attributes explain what record they are associated with. With either approach, we only seem to get half of the story, and maybe we need the whole thing. Not sure if that can be achieved without adding new translation strings, though. Also, as noted above, I think the work here may relate in some ways to #2999, which is going through some conversation to find the best solution for providing unique IDs to all checkboxes. Maybe we should put this on hold until that work is done in case we can leverage the results. What do you think?

@maccabeelevine
Copy link
Member Author

the bigger question is whether it's okay to replace an action-oriented label with just a title. Do we need to combine the two, e.g. "Select %%title%% for renewal"? I realize that making that change would require a significant amount of retranslation work, so it's not ideal... but losing the action label entirely also seems less than ideal.

Personally I do not think the checkbox aria-label should say what the checkbox is for, like "Select %%title for renewal", since for many screens there are several actions you can do with the selected record(s). It would also be repetitive over each record. I do see there is a general translation string

select_item = "Select item for further action"

but IMHO the idea of "further action" is inherent in any checkbox of this kind (i.e. pertaining to a record). With just the title text in the aria-label, NVDA pronounces it as "%%title%% checkbox not checked" which seems the best solution.

That said I am probably wrong and @metageeky would know better.

Let me also draw your attention to #2999, which I believe is work in progress on code that's meant to be a foundation for solving this problem... but I think it's currently a bit stalled and needs more work.

It looks like #2999 has unstalled so I can wait for it to finish.

@demiankatz
Copy link
Member

Good point regarding the possibility of multiple actions, @maccabeelevine. As you say, let's see what wisdom @metageeky can impart regarding best practices for this situation. :-)

@demiankatz demiankatz removed this from the 10.0 milestone Apr 30, 2024
@demiankatz
Copy link
Member

@maccabeelevine, I'm taking the 10.0 milestone off this once since it doesn't seem to be moving quickly, and I'm not sure if we'll get it done in time for that release. Obviously, still happy to merge it if it's done in time -- I just don't want to make the slate of scheduled work for the release larger than necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants