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

fix(core/form/inputs): fix issue with tabbing to popover toolbar buttons in PT-input #5057

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

skogsmaskin
Copy link
Member

@skogsmaskin skogsmaskin commented Oct 24, 2023

Description

There is currently an issue with reaching the annotation and inline object edit toolbar buttons with the keyboard.

This will make sure that when these toolbars shows, it will be possible to press Tab in order to get to the buttons with the keyboard.

Also added component tests for this.

What to review

That you are able to tab into the buttons when the toolbar is showing.

Notes for release

  • Fixed an issue with keyboard navigation to edit buttons for annotation and inline objects in the Portable Text Input.

@vercel
Copy link

vercel bot commented Oct 24, 2023

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

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Nov 1, 2023 9:22am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2023 9:22am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Nov 1, 2023 9:22am

@skogsmaskin skogsmaskin changed the title Fix/pt input tab to toolbar fix(core/form/inputs): fix issue with navigating to toolbar buttons with keyboard Oct 24, 2023
@skogsmaskin skogsmaskin changed the title fix(core/form/inputs): fix issue with navigating to toolbar buttons with keyboard fix(core/form/inputs): fix issue with tabbing to popover toolbar buttons in PT-input Oct 24, 2023
@github-actions
Copy link
Contributor

No changes to documentation

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2023

Component Testing Report Updated Nov 1, 2023 9:26 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 26s 12 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 5s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 12s 3 0 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 11s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 30s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 14s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 7s 21 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 12s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 7s 3 0 0

…th tab

It should be possible to hit Tab and get to the annotation and inline block edit toolbar buttons.
…uttons

This will test that we can use Tab to get to the edit toolbar buttons for annotations and inline objects in the PT-input.
Copy link
Contributor

@robinpyon robinpyon left a comment

Choose a reason for hiding this comment

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

Thanks @skogsmaskin, this is a huge improvement.

Just had a look and noticed a few things:

  1. When pressing TAB for the first time over an annotation, it will try focus the next element briefly before then focusing buttons within the popover.

If you look at this video and pay attention to the 'No items' box, you'll see what I mean:

Screenflick.Movie.30.mp4

I'm very certain this is the case due to buttonRef.current.focus() being wrapped within a setTimeout. This does seem like a bit of a smell!

One possible alternative is that rather than focusing the button directly, we create an invisible anchor element (with tabIndex=-1), and then on TAB:

  • initially focus the anchor element. Since it has tabIndex=-1, it will be ignored in sequential keyboard navigation
  • the browser will then 'naturally' tab to the next button in the list

Here's an example video with that approach:

Screenflick.Movie.31.mp4
  1. This popover doesn't trap focus – making it very easy to accidentally tab or shift-tab away completely out of the form. We should probably trap focus here.

  2. Focus isn't restored when closing the popover – ideally, closing the popover with ESC returns you exactly where you were.

We could probably punt on 2 + 3, but should capture these if we do.

@RitaDias
Copy link
Contributor

@skogsmaskin I'm assuming that this PR can be closed? 👀

@RitaDias RitaDias removed the request for review from a team June 27, 2024 06:15
@hermanwikner hermanwikner removed their request for review July 10, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants