Skip to content
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

Adds a tabindex to the right info panel's main element so it can be f… #1171

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

LlGC-jop
Copy link
Contributor

…ocused regardless of whether it has focus-able content

Fixes #1069

…ocused regardless of whether it has focus-able content
Copy link

vercel bot commented Oct 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 11:14am

@Saira-A
Copy link
Contributor

Saira-A commented Oct 24, 2024

Looks good to me! A couple of additions :

  • When the window size decreases enough, the side panels disappear, and the 'More Information' panel is replaced by a box that opens when the information icon in the footer is selected. However, the box currently also doesn’t receive focus, so it remains non-scrollable if there are no links to focus on inside. It would be helpful for focus to be applied in the same way.
  • If the right panel doesn’t have any text overflow (e.g. ), the panel is still focusable, but there’s no interaction available since there’s nothing to scroll. It might be better to conditionally apply focus to the panel only when there is overflow content.

Copy link
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give this a more thorough test after @Saira-A's suggestions have been discussed, but in the meantime, I had one more small question (see below).

… make elements articles so they'll be readable by screenreaders.
@LlGC-jop
Copy link
Contributor Author

I've updated the elements to be articles and changed it so that whenever the size changes the heights will be compared, and set things accordingly

Copy link
Contributor

@Saira-A Saira-A left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, that works as expected. For the more info dialogue, it might be more intuitive to focus on the metadata before the close button rather than after it since the button is underneath, but I'm not sure which approach would be better from an accessibility perspective.

@demiankatz
Copy link
Contributor

I agree that it might be better to focus the top of the dialog instead of the bottom, though I could live with this if a more robust solution is impractical. I wonder if this behavior is related to #1163. Could it be that if we add article to the list of focusable elements (or come up with some other mechanism for including this element) it would sort things out?

@demiankatz
Copy link
Contributor

@LlGC-jop, just checking in since you've been active on other PRs, but this one has been quiet for a while: any thoughts on the above comment? (If not, no worries -- just making sure this didn't get lost!)

@LlGC-jop
Copy link
Contributor Author

Sorry @demiankatz, been getting a lot of notification emails and only just figured out how to reduce it to ones relevant to my issues/PRs :)

#1163 includes tabindex=0 as a selector so I think that'll cover any instance where we want an article element to be focusable, or any other element for that matter.

I think we should create a new issue to cover all dialogues and which element has focus on open. This would be more specific and have less overlap with the right panel side of things in this issue.

Perhaps a data attribute on the dialogue to specify an ID to focus on, then the main Dialogue.ts file can check for that and set focus on open?

@demiankatz
Copy link
Contributor

Thanks, @LlGC-jop, makes sense to me! Would you like to open a follow-up ticket, or should I?

@Saira-A, do you have any concerns about merging this as-is and addressing the rest separately?

@Saira-A
Copy link
Contributor

Saira-A commented Oct 31, 2024

Thanks, @LlGC-jop, makes sense to me! Would you like to open a follow-up ticket, or should I?

@Saira-A, do you have any concerns about merging this as-is and addressing the rest separately?

Fine by me :)

Copy link
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is safe to merge, then! I've opened #1189 to capture the idea about specifying preferred focus within dialogues with a data element or equivalent.

@demiankatz demiankatz merged commit 8f8a294 into UniversalViewer:dev Oct 31, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Accessibility issue: keyboard users potentially stopped from accessing sidebar
3 participants