Repository navigation
popover: Add loop prop to non-modal Popover.Content - #4140
jafari1995 wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: baa3400 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Checks still haven't run on this branch. It's up to date with main, and the non-modal Tab test passes locally. Could someone approve the workflows? |
modal is false
chaance
left a comment
There was a problem hiding this comment.
I agree that this is probably the correct behavior, but it's also a big enough behavioral change that I'd consider it breaking and something we should ship in a major release. I think for now we should probably expose loop as a prop that consumers can override (for non-modal popovers only; no-op for modal since focus is trapped). In v2 we can flip the default to false.
|
Supporting the direction of exposing In if (!loop && !trapped) return; // L210
...
if (!event.shiftKey && focusedElement === last) {
event.preventDefault(); // L232 — runs before the `loop` check
if (loop) focus(first, { select: true });
}The So for the non-modal case ( Two suggestions for the API:
|
modal is falseloop prop to non-modal Popover.Content
|
Reworked it so nothing changes by default. |
Summary
Non-modal popovers always loop Tab because
FocusScopealways getsloop. Changing that by default is breaking, so this adds aloopprop toPopover.Contentinstead (defaulttrue, same as today). Withloop={false}, Tab from the last control moves focus out of the popover. Modal popovers ignore it and keep looping.Fixes #4128
Test plan
loop={false}lets Tab leave a non-modal popoverloop={false}