Make keyboard activation consistent#1164
Conversation
…ptions for anchor behaviour
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@LlGC-jop, the gallery portion of the problem should be addressed by IIIF-Commons/iiif-gallery-component#20 -- if you have time to try this out, I'd welcome a review so we can get it merged and released so we can incorporate it into UV. There is work in progress on the left panel thumbnail activation in #1155. In that implementation, spacebar does not activate the thumbs, because it instead pages down through the thumbnail list. I can potentially see some value in that approach, but I'm not really sure which is better. In any case, we'll need to reconcile the work in #1155 with the work here. Do you mind taking a look at that PR? If that solution is acceptable, it might be best to merge that first (to get a smaller, more granular commit) and then resolve conflicts over here. If there are problems, maybe it would be easier to close that one and focus on the version here. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LlGC-jop. I have a few comments below, but they're really all the same comment: there seem to be a few places where you are setting the new "anchor as button" argument to true, but you don't need to, because those elements aren't anchors. Maybe this is intentional, because you want to future-proof in case the element types are changed later... in which case I'd rename anchorAsButton to treatAsButton or something more generic to prevent possible future confusion. However, I think my preference would be not to pass extra arguments unless they're necessary, since having sequences of unlabeled booleans generally makes the code less readable. :-)
My apologies if this is all truly necessary and I'm just misunderstanding something!
|
@demiankatz quite right, I'd initially done the code to make the anchors work, and then decided to try changing the elements to the correct type, just to see if there were any knock-on styling issues. There weren't, but I didn't go back and change the function calls, which I'll do this afternoon. re #1155, the change I made was to the react component and should ensure they're only activated on space or enter. I'll take a look at that PR in a bit and see what's changed. As for the gallery, I'll fork it and see how it works - my plan was a tabindex of -1 as well :) |
|
Thanks, @LlGC-jop, this all sounds good to me -- let me know if/when I can be of further assistance. :-) |
|
Hi @demiankatz , I've made the changes necessary, and left a comment on #1155 about what to do with the thumbnail code. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the latest chhanges, @LlGC-jop -- looks like all of my code feedback has been addressed, and the diffs are now a bit easier to read!
We shouldn't merge this until testers have had a chance to try everything and we've finished discussing the other PRs that interrelate with this, so I'll just make this review a "comment" for now... but I'd certainly encourage testing to go forward; I don't expect too much more to change here after this point.
|
I've merged dev into this PR and resolved the conflicts, keep the changes from #1155 |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks again! From my perspective, this is ready to go as soon as it gets the thumbs-up from testers. We can deal with the issues related to the iiif-gallery-component separately, since that work stands alone.
|
I have re-tested this. It fixes #1157. It fixes #1084 also. But I agree with @LlGC-jop in this issue on which he commented in the related issue #1155:
I agree that these are buttons rather than links and ideally should respond to spacebar as well as return key. |
|
Thanks, @scoutb-cogapp, I've opened #1176 to revise the thumbnail keyboard behavior as you suggest. If somebody wants to review that, I can merge it once it's approved -- though if @LlGC-jop would prefer to incorporate the fix here in a more elegant way, I wouldn't be offended and could simply close my version. |
|
@LlGC-jop and @demiankatz , I have tested the fix, and it's working smoothly with both the Space bar and Enter key on my keyboard. Thank you @LlGC-jop and @demiankatz. |
|
Thanks, @LanieOkorodudu -- I will merge this now! |
Fixes #1084
Fixes #1157
All buttons and pseudo-buttons that previously weren't able to be activated via enter/spacebar should now be able to be.
I've modified Panel.ts:onAccessibleClick() to check whether the key is either enter or space and in the case of the latter, prevent the default action (scrolling the page).
It also now has a fourth arg to specify whether an anchor should be treated as a button i.e. spacebar should activate it. This is for things like tabs where they're an anchor but work like a button.
Some elements that were
<div>or<a>have been changed to<button>.I've also made the thumbnails in the left panel work with space+enter, however the gallery view wasn't possible. For some reason you can't even focus inside it, but as it's the iiif-gallery-component I've not looked into it.