Improve accessibility for checkboxes

[elsenhans]

In the list of results and list of favorites, the label for the checkbox was adapted from "Select Page" to "Select all entries of the page". This makes labels the function of the checkbox more clear and understandable.

The titles of the record in the list of results and list of favorites are now read along with the focused checkbox. People who focus the checkboxes by tab and use a screen reader do know about the title of the record already if they are able to select the checkbox.
The same reference to the title has been added to "Save to List", "Add to Book Bab" and "Context" buttons in the list of results.

The same reference to the title has also been added to the lists of account functions.
In addition, the checkboxes for the account functions have been supplemented by an aria-label, which names the possible function, for instance: "Select item for canceling holds or recall".

Since there is no data for the other RecordDriver in our index, the adaption is only possible for the DefaultRecord.
Also I could not activate the checkboxes in the account functions (except for the favorites), because information is missing in the data for it.

For activating the checkboxes or functions I activated in config.ini:

[Site]
showBulkOptions=true
showBookBag = true
bookbagTogglesInSearch = true

TODO

• Add note about cleaning up select_page translation to appropriate "update translations" JIRA ticket for the release where this gets scheduled.

[demiankatz]

Thanks, @elsenhans! I haven't done an exhaustive review yet, but I found a few issues that seem worth discussing, and a few problems that are widespread (lots of attributes being set without having escapeHtmlAttr called on them -- we can't assume that record IDs are HTML-safe). I thought it best to stop after looking at a few files so we can begin the discussion. I'll take a closer look at the remaining changes after we've moved things forward a little bit.

[demiankatz]

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.

[elsenhans]

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.

[demiankatz]

That makes sense, but should we just move it outside of the label tag rather than deleting it entirely?

[elsenhans]

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)

• the with_selected part is misleading, so we removed that
• also we have adjusted the translation for the part of the label which is still there: select_page = "Select all entries of the page"

Does that make sense for you @demiankatz ?

[demiankatz]

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).

[crhallberg]

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.

[demiankatz]

@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_selected if 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?

[elsenhans]

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?

[demiankatz]

Thanks for the language file update. I think you may have meant to tag @ckaz here.

[elsenhans]

@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."

[demiankatz]

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 $cartId on the page. Maybe something like RecordDriver/DefaultRecord/core.phtml needs to be adjusted to include an ID to avoid inconsistencies.

[elsenhans]

@RLangeUni do you know about that problem in our environment?

[elsenhans]

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.

[demiankatz]

$cartId is the value used by the Javascript-based cart system to represent the item -- in the format backendId|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.

[elsenhans]

@RLangeUni do you know how we handle that? Do you have an idea how to solve this?

[robertlange81]

Indeed in finc we are using an extra attribut in cart/contents. I added it in commit c42074dde3.

[robertlange81]

But we could ignore the buttons / leave them out for a later PR and concentrate on the checkboxes only.

[robertlange81]

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).

[elsenhans]

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 ?

[demiankatz]

I don't think that helps -- the issue is that aria-describedby needs to refer to an HTML element ID, but $cartId is 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.

[crhallberg]

Approved, once Demian's concerns have been addressed.

[crhallberg]

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.

[demiankatz]

Some further suggested revisions here. Maybe we should rename this to getUniqueIdWithSourcePrefix to be very specific about what is happening, and then create a new method below for use in the templates:

public function getUniqueHtmlElementId()
{
    return preg_replace("/\s+/", "_", $this->getUniqueIdWithSource());
}

After doing a bit of research, I see that HTML5 element IDs support nearly all characters, but not whitespace -- so to be on the safe side, I think when we use a record ID as an element ID, we should ensure that whitespace is eliminated. (Hopefully the odds of an ID collision caused by replacing whitespace with underscores are small enough that we don't need to worry... but if we ARE worried, we could also do something different like 'record_' . md5($this->getUniqueIdWithSourcePrefix()) to more reliably guarantee uniqueness. This would also offer a nice hook if people wanted to implement different patterns for element ID generation in their own code.

[RLangeUni]

Changed that. Should we use the escapeHtmlAttr helper directly within the record view helper and remove escaping of in templates?

[demiankatz]

Thanks @RLangeUni! I don't think auto-escaping the ID will work, because in some cases we're passing the ID as an array value to the URL helper, which will also automatically escape it -- and we don't want to end up with double-escaped values. We could add a flag parameter for escaping, or have two different method names for escaped vs. unescaped versions, but that seems potentially overcomplicated. It might be tidier to refactor more of the templates to use the htmlAttributes() or makeTag() view helpers for constructing tags with lots of attributes from arrays, and then all the escaping is more cleanly built-in and there's less potential confusion about what is or is not escaped. (But that's a project for another day, I think :-) ).

[demiankatz]

Thanks, @RLangeUni -- I've taken another look and left a few more comments.

As you say, there do seem to be a few different things going on in this pull request, and it's getting a little hard to keep track of. Maybe we should consider splitting it into smaller parts so we can address all the issues independently (and perhaps merge some of the easier changes sooner). If you need help with the process of breaking things apart, let me know; I might be able to assist.

[demiankatz]

I don't think that adding an element ID here in the cart/contents.phtml template is going to help to label buttons rendered by record/cart-buttons.phtml. The contents template only renders when a user opens the cart, whereas the buttons render in the main page as part of the record view or search results. Thus, the IDs being referenced won't exist most of the time when they are referenced. Additionally, if we tried to put these IDs on the search result listings or record view, then we would end up with duplicate IDs.

[RLangeUni]

It was my intention to transfer on the cart page, but it is buggy indeed. I suggest we leave the cart changes out for a later PR.

[RLangeUni]

I restored previous changes in result-list, cart-buttons and toolbar (e8e6c79). Although cart/content could stay, because it applies for the cart page and its checkboxes. What do you think?

[demiankatz]

I think the thing to consider when adding the ID to cart/contents is whether that will lead to duplicate IDs on the page; if we're only going to use cart IDs inside the cart, then it's fine. But if we're going to have them in other places, then we'll run into duplication when we add an item to the cart and then open the cart contents in a lightbox, since the same ID will be present in two different parts of the DOM. Maybe we should consider a parameter to prefix the ID so we can ensure ID uniqueness in different contexts -- i.e. one prefix for associating "add to cart" buttons with search results, and a different prefix for associating cart content checkboxes with cart content listings.

[RLangeUni]

Thanks for your advice. I noticed that problem. It should be possible to open record view from result list like HMT, so I tend to separate between lightbox and non-lightbox without prefix instead of a contextual differentiation?

[demiankatz]

I can't think of a scenario where favorites and holdings would get rendered side-by-side -- it's not possible in "standard" VuFind, though I suppose it could be done as a local customization somewhere. Using a prefix for lightbox content is probably helpful, as long as we never develop a system with multiple or stacked lightboxes. I still slightly prefer prefixing based on context rather than lightbox status because it seems more semantically meaningful, but it's also probably more work to implement.

If you wanted to account for lightbox status in getUniqueHtmlElementId, I think it could probably be done by accessing the view model from inside the helper so you can check the inLightbox property. I think $this->getView() might do the job, but I can't remember if that gives you the view object or only the renderer.

[RLangeUni]

It is only the renderer, but by using the view model helper within the record view helper we could get this information and the current template as "context". The lightbox prefix should be useful for the edge case of rendering a modal of the same page (Cart/Home on new tab + click on items).

I wonder if we should use either $idPrefix OR a context string based on the template as implemented yet in getUniqueHtmlElementId. It seems redundant otherwise.

[demiankatz]

Yes, very good point about the edge case of opening a lightbox that duplicates the current page content!

Creating a prefix based on the template being rendered and the lightbox status might be enough to make everything work correctly, though explicitly passing in a context string instead of relying on the template name might be a little more robust if parts of the code end up getting refactored to smaller sub-templates in future. (Though that comment is not based on a recent examination of how the parts fit together, so maybe I'm not fully understanding what you propose).

[RLangeUni]

Using the template name would be more generic, I thought. How should we name the context string from and where should it be set (in the template)? The context variable could be an array as I understand it, that could cause trouble.

[demiankatz]

I was just thinking that if we only have two or three contexts, the template that generates the unique ID would just pass a string like result_ or cart_ or list_ to the call to get the unique HTML ID. If we have distinct templates for all these scenarios, then it's easy to do. If there are any shared templates, then the context string will have to be passed in to the template from a higher level where the context can be identified.

[demiankatz]

If we're going to change the values passed to the cart-buttons.phtml template, maybe it would make the most sense to just pass in the record driver so more granular decisions can be made within that template.

However, changing this part of the code is fairly backward-compatibility-breaking and might not be a good change to make in a minor release; we might want to reschedule this part of the work for release 10.

[demiankatz]

@elsenhans / @RLangeUni, this PR seems to have gotten a bit stuck -- there are still a lot of unresolved conversations, and conflicts are beginning to accumulate. I thought it would be helpful to begin extracting parts of it into smaller and more manageable PRs. I started by opening #2978, which takes care of removing the unnecessary with_selected string independent of other changes. Do you have any thoughts on how we can begin to extract other improvements from this PR separately so we can make more progress on completing it?

[robertlange81]

Thanks for pushing it further. I suggest to add one PR for our new helper methods getUniqueHtmlElementId and getUniqueIdWithSourcePrefix in the record view helper. But without the generic part and the viewModel property. Instead by an additional context param in the method as I understood you correctly.

Another PR could be for using the value of getUniqueHtmlElementId and setting the $describedById within the templates. I am not quite sure how to determine the context.

A fourth PR could be for simple string / helper texts: select_all_on_page, select_all_on_page. Alternatively as part of
2978.

[demiankatz]

Thanks, @robertlange81, that all makes sense. I should be able to get #2978 merged next week. Let me know if you have time to set up some or all of these PRs, or if you'd like me to look into it. I may have some time next week to do at least one or two.

[RLangeUni]

I try to set all up till the end of next week.

[demiankatz]

@RLangeUni, since most of the functionality from this PR has already been merged to dev or is represented by #2982 and #2999, I'm going to close this PR now. The only piece that hasn't been refactored somewhere else is the application of the new helpers added in #2999. Since that will probably take a somewhat different form than what is found here, I've just added a TODO checkbox to #2999 to remind us to figure out next steps after resolving the conversation about the helper methods. We'll still have all this conversation linked for reference if we need it, but I think it's best to remove this from the active PR list to keep things clean. If you want to discuss next steps in more detail, let's take that over to #2999. Thanks again for all the work breaking this apart!