Fix accessibility of record tabs in v11

[ThoWagen]

As discussed in #4901 this is just a quick fix that makes it possible to select tabs using the enter key.

[demiankatz]

@ThoWagen, I haven't done hands-on testing yet, but see below for one small question regarding a comment:

[ThoWagen]

With this VUFIND-1729 will actually not be resolved completely. It only makes it possible to use the keyboard to select elements. But it does not completely fix the accessibility of the record tabs.

[demiankatz]

With this VUFIND-1729 will actually not be resolved completely. It only makes it possible to use the keyboard to select elements. But it does not completely fix the accessibility of the record tabs.

True -- I have moved the "resolve VUFIND-1729" TODO back over to #4901, and I have set the appropriate fix version on the ticket.

[demiankatz]

Thanks, @ThoWagen -- after a more careful reading of the code, one last suggestion:

[sturkel89]

@demiankatz asked me to test this PR. I'm entering my observations here as a comment.

Release-11.0 behavior

In the release-11.0 branch, it's possible to tab through an item record, including tabbing through each record tab, and then open a tab by clicking Enter. However, this only works once; after that, you can arrow back and forth between the record tabs but Enter will not open them. Record tabs is broken in release-11.0, in terms of navigability via keyboard.

Test branch behavior

In the test branch, you can consistently use the Tab key to get to the first record tab, then use the arrow keys to switch between selected record tabs, and the Enter key to open that record tab. You can do that multiple times without failure. So, this branch fixes something broken!

My opinion

This solution feels unintuitive to me. Elsewhere in item records, I can use the Tab key to navigate through multiple options of the same type (e.g., Cite/Print/Export/etc. options, or multiple subject headings). However, I can't use the Tab key to navigate between record tabs; just for this one part of the record, I have to switch to arrows. Why the difference? Would a screen reader user know to switch to arrows? Would they even know there were other record tabs available, if the screen reader just read the name of the first one aloud and then tabbing led to Similar Items?

Can we get feedback from someone who could provide a more informed opinion on the accessibility of this solution?

[demiankatz]

@sturkel89, I was able to find some formal advice for keyboard accessibility of tab panels -- see the "Tab panel" section of the WebAIM Keyboard Accessibility page. It sounds like the behavior implemented in #4901 exactly matches the guidelines. The implementation here falls short because the expectation is that the panel will immediately update, but it is necessary to press Enter here instead... but if that's the best we can easily do without the full rewrite in #4901, it seems like an incremental step closer to correctness, and is better than being completely broken.

[demiankatz]

@ThoWagen, I had an idea and did a little more refactoring and fixing here -- and now I think this is behaving more or less the way we want it to. Do you mind reviewing my changes and letting me know what you think?

(I just refactored the event handler to a function, hooked the function to both click and focus events, and added an explicit focus call when the initial tab is activated).

[ThoWagen]

It definitely is an improvement. You still have to press enter on tabs, that can not be loaded via AJAX.

In #4901 I fixed that by simply loading all of the non-AJAX-tabs when loading the record for the first time. I don't think we should reload the page when switching through the tabs with the arrow keys.

Do you think, we should do it here as well?

[EreMaijala]

@ThoWagen Just preloading tabs isn't enough for the CollectionList tab in Collection module. It actually expects to have /CollectionList in the URL. Any filtering operations etc. will fail otherwise.

[ThoWagen]

@EreMaijala I'm not familiar with the collections and was not aware of that.Can you show, how I can reproduce that problem in the test environment? What changes are required in the configuration and what URL should I call?
Nevermind, I found out how to reproduce that.

When I call "/Collection/topcollection1" it behaves as on the dev branch. In both cases it looks like this:

This is probably an unrelated problem?

[EreMaijala]

@ThoWagen I reported it yesterday in #4901. And yes, the layout problem is a different issue.
You may have to enable more tabs and make something else the default in CollectionTabs.ini. Then when you switch to the Collection Items tab and try to filter by something, you won't get the tab back.

[EreMaijala]

@ThoWagen It would be possible to refactor the tab so that it doesn't require any reloading by adapting the JS-loaded search results to support it, but it'd require some effort.

[ThoWagen]

@EreMaijala Ok, I will see what I can do for #4901. But I would leave this PR as is then.

[demiankatz]

Regarding the collections layout problem, see #4937 for a fix in progress (it's not quite right as of this writing, but if you merge it in, it at least will make the collections page usable for testing).

Regarding URL requirements for tabs, I complained yesterday in #4901 that changing the tab no longer changes the URL, so you can't bookmark specific tabs there. I know there is a mechanism for changing the full URL in the browser (not just the hash part), since single-page apps use it. I wonder if it's possible for us to put a data property on our tabs containing the full URL of the tab, and then use that to update the browser address when tabs are switched. That would be more elegant than the current hash-based solution and might also solve the collection problem. Of course, that work is probably best confined to #4901 -- I'm not trying to make this "small fix" hugely more complicated.

Regarding the work here, I think the last question is whether the ARIA labeling is adequate, or if we should consider improving it to make this more fully accessible. I plan to do a little more experimentation this morning, so I may comment further in a few minutes.

[EreMaijala]

@demiankatz I think that's a good idea! We're already doing the history manipulation in several places (including the JS-loaded search results).

[demiankatz]

Okay, I've done a bit more tinkering with this and have also inspected the generated markup. Now that #4938 has been merged and all of the default tabs are AJAX-capable, the experience on record pages seems pretty good. If you turn on collections and hierarchy trees, things get worse, though, because of the issues @ThoWagen already mentioned. The Context tab on the Record page introduces some weird behavior, and the Collection view is kind of a mess (not only do you need to hit enter to load the non-AJAX tabs, but there's also something strange about the tab selection where the tab list loses focus if you hit the arrow keys enough times).

I'm no expert on ARIA, but it does at least appear that the appropriate roles are being used in the appropriate places (though the known problems with non-AJAX tabs are unavoidably going to bend or break the model until we better address them).

Bottom line: this isn't perfect, but I do believe it is significantly better than what we have without these changes. For the majority of common use cases, things will be completely fixed -- but we definitely need to give some attention to the Collection edge cases in #4901.

What does everyone think? Should we merge this now as an incremental improvement, or should we do any further polishing first?

[ThoWagen]

I would merge this as is.

[demiankatz]

Edit: I left this comment on the wrong PR. I will move it to the right place now. :-)

[demiankatz]

Thanks, @ThoWagen -- merging now!