Skip to content

VUFIND-1673 Set unique labels for checkboxes #3552

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.

Sign up for GitHub

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

Open
maccabeelevine wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

VUFIND-1673 Set unique labels for checkboxes #3552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5a2d79b
VUFIND-1673 Set unique labels for checkboxes
maccabeelevine Apr 2, 2024
0ea1a8b
Handle checkedout.phtml checkboxes
maccabeelevine Apr 2, 2024
2d5c7da
Merge branch 'dev' into checkbox-label
maccabeelevine Apr 23, 2024
71ae34f
Strip tags on title
maccabeelevine Apr 23, 2024
ecc4c52
Merge branch 'dev' into checkbox-label
maccabeelevine Oct 22, 2024
2e48d0a
Use new getUniqueHtmlElementId
maccabeelevine Oct 22, 2024
6203cb0
Fix spacing
maccabeelevine Oct 22, 2024
1846426
Update bootstrap5
maccabeelevine Oct 22, 2024
75a862d
Use aria-labelledby on checkedout.phtml
maccabeelevine Oct 23, 2024
e5db19d
Fix bootstrap5
maccabeelevine Oct 23, 2024
df57607
Merge branch 'dev' into checkbox-label
maccabeelevine Jul 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion module/VuFind/src/VuFind/View/Helper/Root/Record.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,8 +421,9 @@ public function getCheckbox($idPrefix = '', $formAttr = false, $number = null)
{
$id = $this->driver->getSourceIdentifier() . '|'
. $this->driver->getUniqueId();
$titleHtml = $this->getTitleHtml();
$context
= ['id' => $id, 'number' => $number, 'prefix' => $idPrefix];
= ['id' => $id, 'number' => $number, 'prefix' => $idPrefix, 'titleHtml' => $titleHtml];
if ($formAttr) {
$context['formAttr'] = $formAttr;
}
Expand Down
2 changes: 1 addition & 1 deletion themes/bootstrap3/templates/myresearch/checkedout.phtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
<?php if (isset($ilsDetails['renewable']) && $ilsDetails['renewable'] && isset($ilsDetails['renew_details'])): ?>
<?php $safeId = preg_replace('/[^a-zA-Z0-9]/', '', $ilsDetails['renew_details']); ?>
<label>
<input class="checkbox-select-item" type="checkbox" name="renewSelectedIDS[]" value="<?=$this->escapeHtmlAttr($ilsDetails['renew_details'])?>" id="checkbox_<?=$safeId?>" aria-label="<?=$this->transEscAttr('select_item_checked_out_renew')?>">
<input class="checkbox-select-item" type="checkbox" name="renewSelectedIDS[]" value="<?=$this->escapeHtmlAttr($ilsDetails['renew_details'])?>" id="checkbox_<?=$safeId?>" aria-label="<?=$this->escapeHtmlAttr($ilsDetails['title'])?>">
Copy link
Member Author

@maccabeelevine maccabeelevine Oct 22, 2024

Choose a reason for hiding this comment

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

I don't see a clean way here to get the record driver, to pass to the Record view helper for getUniqueHtmlElementId. I could use $ilsDetails['item_id'] which in my testing is the same value, but I believe there are cases where that might not be. Alternately maybe I can use $safeId and generate the same value in account-entry.phtml, but I don't know if that adds any value.

If there is not a way to get getUniqueHtmlElementId here, then I should revert the changes to account-entry since they don't have any purpose.

Copy link
Member Author

@maccabeelevine maccabeelevine Oct 22, 2024

Choose a reason for hiding this comment

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

And once we figure out this screen, I can apply the logic to the other account pages with checkboxes.
#3552 (comment)

Copy link
Member

@demiankatz demiankatz Oct 22, 2024

Choose a reason for hiding this comment

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

In this template, I believe that $resource is the record driver. Does that help?

Copy link
Member Author

@maccabeelevine maccabeelevine Oct 23, 2024

Choose a reason for hiding this comment

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

Yes, thanks! Got this page working with aria-labelledby as well now. Not to presuppose the final decision, but at least the approach is consistent now in this PR for the pages that the Moravian report originally cited.

</label>
<input type="hidden" name="selectAllIDS[]" value="<?=$this->escapeHtmlAttr($ilsDetails['renew_details'])?>">
<input type="hidden" name="renewAllIDS[]" value="<?=$this->escapeHtmlAttr($ilsDetails['renew_details'])?>">
Expand Down
3 changes: 2 additions & 1 deletion themes/bootstrap3/templates/record/checkbox.phtml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

@demiankatz demiankatz Apr 2, 2024

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

@maccabeelevine maccabeelevine Apr 23, 2024

Choose a reason for hiding this comment

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

Done

<span class="checkbox-icon"></span>
<?php if (strlen($this->number ?? '') > 0): ?><span class="sr-only"><?=$this->transEsc('result_checkbox_label', ['%%number%%' => $this->number]) ?></span><?php endif; ?>
</label>
Expand Down