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

Allow setting constrained tabbing behaviour in useDialog hook #57877

Closed
getdave opened this issue Jan 16, 2024 · 3 comments · Fixed by #57962
Closed

Allow setting constrained tabbing behaviour in useDialog hook #57877

getdave opened this issue Jan 16, 2024 · 3 comments · Fixed by #57962
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@getdave
Copy link
Contributor

getdave commented Jan 16, 2024

the useDialog hook currently does not provide developer control over whether the dialog has constrained tabbing or not.

Instead this behaviour is conditionally set based on the focusOnMount prop. I spoke a bit with @andrewhayward about this away from Github and he suggested the following:

Ideally useDialog wouldn't assume modality, and it would be something to be opted into, but that's not going to happen at this point. It's definitely weird that the behaviour is tied to focusOnMount, regardless; would be good to break that association, whilst also somehow maintaining backwards compatibility.
Something like this should work...

type DialogOptions = {
  focusOnMount?: Parameters< typeof useFocusOnMount >[ 0 ];
  constrainTabbing?: boolean;
  ...
};

function useDialog( options: DialogOptions ): useDialogReturn {
  ...
  const {
    focusOnMount,
    constrainTabbing = focusOnMount !== false,
  } = options;
  ...
  return [
    useMergeRefs( [
      constrainTabbing ? constrainedTabbingRef : null,
      focusOnMount !== false ? focusReturnRef : null,
      focusOnMount !== false ? focusOnMountRef : null,
      closeOnEscapeRef,
    ] ),
    ...
  ];
}

If no options are passed in, constrainedTabbingRef will continue to be used
If focusOnMount is specified, constrainedTabbingRef will continue to be used as before
If constrainTabbing is specified, its value will be used to determine if constrainedTabbingRef is used


@joedolson @jeryj Testing the a11y I noticed that there appears to be a focus trap which is active on the initial creation of the link only.

When you first create the link your focus is trapped within the preview or the editor form and it is not possible to TAB out. Only ESC allows you to escape focus trap.

If you then re-activate the link the focus trap is no longer in evidence.

Here's a video demonstrating what I mean:

Screen.Capture.on.2024-01-12.at.10-06-19.mp4

I'm going to try and look into this but if you have any ideas what could be causing this it would be greatly appreciated.

Update: I suspect this line is the source of the bug:

options.focusOnMount !== false ? constrainedTabbingRef : null,

In inline.js we set conditionally set focusOnMount. This is to

  • allow the Link UI Popover to retrieve auto focus when you trigger the Link UI for the initial creation of the Link.
  • stop the Popover auto-focusing when you place your cursor into an existing link format. This is so you can use arrow keys to move into the link format without having focus "stolen" by the Link UI popover.

However, by doing this we toggle the constrained tabbing behaviour in the useDialog hook. This is why we get different behaviour.

We'll need to work on this. For example, is it suitable to allow Popover to control whether it has a focus trap or not? That would also require modifying useDialog to allow for this override.

Originally posted by @getdave in #57726 (comment)

@getdave getdave added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system labels Jan 16, 2024
@getdave
Copy link
Contributor Author

getdave commented Jan 16, 2024

cc @ciampo @andrewhayward @jeryj

@jeryj
Copy link
Contributor

jeryj commented Jan 16, 2024

Re the focus trap on link creation, this comment is long, but I think explains it: #57726 (comment)

The problem isn't, "it traps focus on initial creation and doesn't on second."
The problem is, "Link editing should be a focus trap, and link preview should not."

The reasoning:

  • Link Preview is something you Tab into. It should not trap focus.
  • Link Edit is a form that gets focus sent to it via an action. When Link editing is exited, it should return focus to whatever opened it (the preview, the text content, or the toolbar).

@getdave
Copy link
Contributor Author

getdave commented Jan 17, 2024

Thanks for clarifying that @jeryj.

Just to say that the Issue is:

  • on initial creation of the link the dialog has focusOnMount set
  • when you re-open the link (e.g. by moving the keyboard cursor into the link format) focusOnMount is not set because we don't want to steal focus from the text and into the link dialog. Instead we allow the user to tab into the dialog.

The problem as I see it is that focusOnMount controls whether there is a focus trap and it's not configurable by a developer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants