Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve accessibility for checkboxes #2874
Improve accessibility for checkboxes #2874
Changes from 6 commits
bfaa2cc8d33a911da832dad01c49c2a2d52fb426c275006362cc90c4bf813bf04eacd703b20d55c0e6d3714004ac42074df85803af03483196e06926fb9a63e8e6c796b2ad39597cd7adb3f73e8957ff684ab077b5fb3d9dd7a49fb954a76520429bad2062bFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing the with_selected text? I think the purpose of this is to indicate that the buttons in this region only impact selected items. I admit that this may not be the most clear way to accomplish that clarification, but I'm not sure if completely removing it makes things better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the label ("| Selection:") is misleading because it refers to the following buttons/checkboxes. It does not refer to the checkbox where the label is. That's why this part of the caption should be removed from the label element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, but should we just move it outside of the label tag rather than deleting it entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the label consisted of 2 parts:
"Select Page | with selected" (or with tokens:
select_page | with_selected)Now the label says:
"Select all entries of the page" (or with tokens:
select_page)Does that make sense for you @demiankatz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "with_selected" part is misleading when it's included as part of the label for the "select all" checkbox, but I think it is helpful to provide context for the buttons. I'm just suggesting you change:
<checkbox><label>select_page | with_selected</label> <buttons>to:
<checkbox><label>select_page</label> | with_selected <buttons>Also, I think "Select all entries of the page" seems very verbose for a short inline checkbox -- we chose "Select Page" to be as concise as possible. Maybe there's a middle ground that's more clear but also shorter. We can't use "Select All" because that might imply "all items in result set" and we need to be clear that it's only "all items on current page." But maybe "Select All on Page" would be a happier middle ground?
Also note that significantly changing an existing translation string can be problematic, because it won't automatically trigger translations of all the other languages. If we really do want to significantly change this label, we might want to rename it as well to ensure that appropriate matching translations are created (e.g. select_all_on_page instead of select_page).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that "with_selected" and misleading and may be poorly translated.I agree that @elsenhans's "Select all entries" is a better label.EDIT: DK helped me see that that "with_selected" is for the buttons and not the checkbox. It does need to stay but maybe wrapping the buttons in an element like a div would make the separation more clear for future development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crhallberg, @sturkel89 and I just looked at some other examples of controls similar to these in different sites. It does seem like we can safely drop the "with_selected" part, since it doesn't really add much clarity. If we get rid of it in both places where it is used, we should also remove it from the language files -- you can do it automatically with
php $VUFIND_HOME/public/index.php language/delete with_selectedif you want to.I think the relationship between the select boxes and the buttons may be more clear if the buttons are disabled when no boxes are checked. That is probably out of scope for this PR, but it might be something we should consider as a follow-up.
Some systems display a message after or below the buttons indicating how many items are selected (e.g. "You have selected X items."). That might be a useful addition. (Again, possibly a follow-up PR rather than part of this one).
In situations where results are numbered, using those numbers for the select all label might be more clear and concise than the other options we have discussed -- e.g. "Select 1-20." However, not every listing in VuFind is numbered, so this idea may have limited applications. Maybe we should consider adding more numbers to help accommodate it. If we can't, though, I at least have some further suggestions for refining your proposed language strings, which I will propose above.
What do you think, @elsenhans?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @demiankatz I did remove "with_selected" from the language files.
To disable the buttons if no checkbox is selected sounds good to me.
With the additional text "You have selected X items" and numbering the results, I am not sure if that would really be helpful. Numbers don't have any connection or information to the record and that's why they do not contain added value. @ckaz what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the language file update. I think you may have meant to tag @ckaz here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ckaz Do you have an opinion on this:
"Some systems display a message after or below the buttons indicating how many items are selected (e.g. "You have selected X items."). That might be a useful addition. (Again, possibly a follow-up PR rather than part of this one).
In situations where results are numbered, using those numbers for the select all label might be more clear and concise than the other options we have discussed -- e.g. "Select 1-20." However, not every listing in VuFind is numbered, so this idea may have limited applications. Maybe we should consider adding more numbers to help accommodate it."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should
$this->escapeHtmlAttr($cartId)to ensure valid HTML.Also note that this template can be rendered as part of the record view as well as in the search results. In the context of the record view, there will be no element with the ID of
$cartIdon the page. Maybe something like RecordDriver/DefaultRecord/core.phtml needs to be adjusted to include an ID to avoid inconsistencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RLangeUni do you know about that problem in our environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the examples I just checked have the ID of the record as
$cartId, that must be always available in the record view. Maybe I do misunderstand something here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$cartIdis the value used by the Javascript-based cart system to represent the item -- in the formatbackendId|recordId-- but it is not necessarily an HTML element ID on the page. In fact, because recordId is not constrained to any particular set of characters, it's even possible that this is not a legal HTML element ID. I think we need a different mechanism for associating these things together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RLangeUni do you know how we handle that? Do you have an idea how to solve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed in finc we are using an extra attribut in cart/contents. I added it in commit c42074dde3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we could ignore the buttons / leave them out for a later PR and concentrate on the checkboxes only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for now there is no unique-id on the detail record page and the aria-describedby within the cart button is useless, but it should do no harm? Eventually we must avoid duplicate ids (I think the case should not occur).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for this PR we can just move the
aria-describedby="<?=$cartId?>"inside the if-condition<?php if (!$cart->contains($cartId)): ?> correct<?php endif ?>"and make further improvements (like getUniqueIdWithSource()) in another PR?What do you think @demiankatz @RLangeUni ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that helps -- the issue is that aria-describedby needs to refer to an HTML element ID, but
$cartIdis an identifier used by the cart's Javascript code. There is no guarantee that a matching HTML element exists anywhere, so in many cases, this is going to be a broken reference that doesn't accomplish anything.