[VUFIND-1729] Fix accessibility for record tabs

[ThoWagen]

This PR is an overhaul of the record tabs logic. It separates the record tab specific logic and general handling of tabs, which is now done by the default bootstrap 5 logic.

By that it fixes the accessibility problems for the record tabs.

As a side effect, jQuery was removed in all of the tabs related logic.

TODO:

• Merge Split JS record logic #4943 first.
• Use the getExtraScripts method of the TabManager to load tab scripts in lists.
• Resolve VUFIND-1729 when merging
• Update changelog when merging

[demiankatz]

Thanks, @ThoWagen, I'll review this as soon as time permits!

This looks like a big enough change that we probably ought to target it to release 12.0. Would you agree?

[ThoWagen]

Yes, it is a big change and I would normally agree to do it in 12.0, but it is fixing a big accessibility problem for screen reader users, since they currently have no way to change the record tabs at all.

If this change is to big for a minor release, I could try to make it at least possible to use the enter key for selecting a tab, even if that is not the default tab behavior for screen reader users.

[demiankatz]

@ThoWagen, if you think there's a relatively light-weight fix we could apply to release-11.0 or dev to get the worst aspects of the problem fixed in 11.0.1 or 11.1.0, please feel free to open a separate PR. If that is not possible (or simply too much work), we could have a conversation on the next Community Call about whether it's worthwhile to make an exception and break some things in a minor release in the interest of fixing a major problem.

[ThoWagen]

The light-weight fix was actually easier than I thought. Here is the PR for it #4903

[dltj]

Wow — I don't feel I'm in any way qualified to comment on the work, but I'm happy to engage with the University of Colorado accessibility lab to ask them to test this when we think it is ready.

[demiankatz]

Thanks for opening #4903 -- I'll test and review that as soon as time permits. In the meantime, I'm locking this in for the 12.0 milestone since that seems to make the most sense.

[sturkel89]

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

Dev-12.0 behavior

Dev-12.0 currently has the same behavior as release-11.0. In the dev-12.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 dev-12.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 record tabs. As you arrow between tabs, the tab is immediately opened! You can do that multiple times without failure. This branch fixes something broken and improves the user experience.

My opinion

As I mentioned in my comment on #4903, I am not sure about the intuitiveness of having to switch to arrows instead of tabs for just this one section of the item record. However, I much prefer the way this version immediately opens each tab as I arrow between them. I think this is improved functionality compared with the other version.

I think MY optimal solution would be for tabbing to work in Record Tabs, and for tabbing to actually switch which tab was open. But I'd like to hear the opinion of an accessibility expert before fully recommending that solution.

[EreMaijala]

The tab pattern in ARIA APG describes the expected behavior with tabs But of course the navigation pattern won't be correct unless the ARIA roles are correct as well and the user identifies the control as a tab list.

Looks like this improves things a lot. However, we can't do all this properly without making the tabs proper tabs that never reload the whole page. Our current non-ajax tab support means that any tab can potentially be a link that actually reloads the page, which sort of breaks everything. Speaking of which, as far as I can see this breaks e.g. the CollectionList tab in Collection module.

[demiankatz]

@EreMaijala, the non-ajax tabs do pose a significant challenge. I'd love to do away with that feature, but for complex Javascript tab content, there is often no simple solution. (I think the main culprit right now is the SimilarItems tab, which embeds the Channels functionality, which loads and bootstraps a whole lot of Javascript... the new mechanism is less complicated than the old, but I still suspect it would take some doing to make it work seamlessly over AJAX).

[demiankatz]

Okay, so maybe non-AJAX channels are not such a big deal -- see #4938.

[demiankatz]

Thanks, @ThoWagen!

I haven't had time to thoroughly review all of this yet, but I looked over some of the files and came up with a few comments below.

I also wonder whether it would be useful to split some chunks of this work into separate PRs -- for example, the refactoring of the comment/rating Javascript into a separate file. If that's too entangled with other changes to easily separate, I'm not trying to create more work for you... but if there are things like that which could stand alone, it might give us a cleaner Git history to do them incrementally (and it might be easier to test and review them thoroughly in isolation).

[demiankatz]

This test reveals that we have lost the functionality that updated the URL hash with the tab name. This was useful, because it allowed us to bookmark specific tabs. Is it feasible to retain that functionality with the new code?

[ThoWagen]

I implemented your suggestion to add the tab to the URL and that fixes this problem and also as you suspected the problem with the CollectionList tab.

[ThoWagen]

Note that the hash-version still works when initially loading a page. So, old bookmarks also still work.

[EreMaijala]

If @demiankatz's suggestion in #4903 is implemented, we just need to ensure that the correct tab is activated also when the URL changes for bookmarking and browser history traversal to work properly.

[demiankatz]

Looks like some conflicts need to be resolved, probably as a result of #4938. Hopefully nothing too difficult to resolve, but let me know if I can be of any assistance!

[demiankatz]

@aleksip, I noticed from your comments in #4796 that you have plans on your roadmap to introduce a section service for tabs. I thought it might be a good idea to be sure you were aware of the ongoing work here so you can comment if there are likely to be problems or conflicts as a result.

[sturkel89]

I've tested this PR in both themes, with Collections and the tree view turned on.

Everything seems to work as expected:

• Tab and Shift-Tab allow you to move between links all over the item record, and between the links and buttons on a given tab
• Right, left, up, and down arrows allow you to move between the tabs on both item record pages and Collection pages
• The contents of the tab and the page URL change as you arrow between tabs, but it's clear that only the tab content is being changed/loaded; the entire page does not seem to reload.

From the user perspective, I think this PR is ready to go.

Thanks for all the work on this, @ThoWagen! I was surprised when I saw that 145 files had been changed - wow!!

[ThoWagen]

@sturkel89 I did not update this with the dev-12.0 branch, so it showed a lot more changes than I actually did for this PR :)

But I'm happy that this works from the user perspective. Thanks for testing!

[aleksip]

@demiankatz Thanks for pointing this PR out. I don't think these changes would cause problems for or conflicts with our planned tabs section. If the section service is merged and proves to be useful there might be opportunities to refactor many things as sections, probably including at least some of this functionality, but that would be completely optional.

[demiankatz]

Thanks for the approval on #4961. I have merged that fix and test update all the way through to dev-12.0, so you should be free to resume work here at your convenience.

[ThoWagen]

I resolved the conflicts. But will also create a separate PR for the removing of jQuery in embedded_records.js.

[demiankatz]

The next step here is to resolve conflicts introduced by #4962.

Since we're about to leave on holiday break, I understand that @ThoWagen may not get to this before the new year -- I just thought it might be helpful to leave this review so we don't forget where we left off!

[ThoWagen]

I resolved the conflicts

[demiankatz]

Thanks, @ThoWagen. I see there's a TODO checkbox about using getExtraScripts. Does that still need to be done? Should I re-review now or wait until more progress has been made?

[ThoWagen]

I forgot that, but did it now. This PR should now be ready for another review.