Adding constrainTabbing prop to useDialog hook
#57962
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tabbing constraint is currently tied to the `focusOnMount` prop in `useDialog'; if `focusOnMount` is not `false`, tabbing will be constrained. Sometimes we want the `focusOnMount` behaviour, without constrained tabbing. This patch adds a separate `constrainTabbing` prop, which implicitly maintains the current behaviour, taking its default value from `focusOnMount`. Otherwise, if set explicitly, tabbing will be constrained based on the value passed.
andrewhayward
added
[Type] Enhancement
ciampo
reviewed
Jan 18, 2024
There was a problem hiding this comment.
Rather that the individual props, I wonder if we should consider a modal boolean prop:
- when
true, the popover constrains tabbing and sets the rest of the document as inert - when
false, the popover doesn't constrain tabbing nor removes the rest of the document from the accessible tree
(cc @mirka )
This sounds ideal to me in theory, and it aligns with the terminology/behavior used for the One caveat is that I don't have too much context on the overall dialog strategy used in our codebase — looking at the Popover/Modal components, some parts are handled with wp-compose hooks while other parts use one-off utils like this That said, at first glance it does seem feasible to add an |
mirka
reviewed
Jan 18, 2024
Trying to think if there's ever a situation where a dialog would want to constrain tabbing, but not be modal. Or, equally, if there's a case for something to be modal but not a dialog. Just wondering if it's worth creating another hook ( |
getdave
reviewed
Jan 18, 2024
There was a problem hiding this comment.
Trying to think if there's ever a situation where a dialog would want to constrain tabbing, but not be modal.
I did some digging here. MDN docs state that a dialog can be non-modal (we knew this).
They also state that for most dialogs, the expected behavior is that the dialog's tab order wraps.
They go on to say that for non-modal dialogs there will have to be a global keyboard shortcut that allows focus to be moved between opened dialogs and the main page.
I also like @mirka's idea of having a isModal prop to handle all this.
The fun thing here is that although the hook is called Unfortunately, without a fair bit of refactoring, I'm not sure as we can safely assume |
|
Given all the uncertainty, it does sound like it would be more manageable to keep things modular so we have the flexibility to combine behaviors as needed. Let's stick with |
I started working on tests for this, but discovered a bug in Essentially, So we either wait for that to be resolved, or we proceed without working tests. |
Scaffold the test and then proceed whilst also raising an Issue to re-enable the tests once fixed? |
|
Size Change: +2.34 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
I've added some tests, but disabled the ones that currently fail (essentially any that expect tab constraint). Will file a new issue to sort that if/when this lands. |
|
Flaky tests detected in e7f4d59. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7646504342
|
ciampo
reviewed
Jan 25, 2024
There was a problem hiding this comment.
Things are looking good, at least when using the component in isolation.
@getdave , would this PR work for the use case that you initially brought up?
Unrelated to this PR, but I noticed that, at least when testing in Storybook, clicking on the <Popover /> component's children closes the popover. The behavior can also be reproduced on trunk. I don't think that's expected ?
|
Testing this now |
getdave
approved these changes
Jan 26, 2024
There was a problem hiding this comment.
In my testing this works well.
I tested using
gutenberg/packages/format-library/src/link/inline.js
Lines 244 to 251 in a241134
I set some test variables and passed them to the Popover and also console.log them so you can see the results of each test.
| focusOnMount | constrainTabbing | Result | Screencapture |
|---|---|---|---|
false |
undefined |
Passed | Video |
true |
undefined |
Passed | Video |
true |
false |
Passed | Video |
false |
true |
Passed | Video |
Left a test nit, but nothing major. Thank you for working on this 🙇
|
@mirka @ciampo don't know if you have any final thoughts, but this seems ready to land.
Can't comment on whether it's expected, but it does seem a bit weird! If it's happening in |
andrewhayward
deleted the
57877/constrained-tabbing-behaviour-in-useDialog-hook
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #57877.
What?
This patch adds a
constrainTabbingprop to theuseDialoghook (and consumes it in thePopovercomponent).Why?
Tabbing constraint is currently tied to the
focusOnMountprop inuseDialog; iffocusOnMountis notfalse, tabbing will be constrained. Sometimes we want thefocusOnMountbehaviour, without constrained tabbing.How?
Tabbing constraint is now controlled by the
constrainTabbingprop. To maintain backwards compatibility/current behaviour, it implicitly takes its default value fromfocusOnMount.Otherwise, if set explicitly, tabbing will be constrained based on the value passed.
Testing Instructions
useDialogandPopovershould continue to work as before.Hooks don't seem to have unit tests, but I should probably add one to
Popover(and maybe a story).