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

feat(dialog, modal, popover, sheet): add options prop to customize focus-trap #11453

Merged
merged 12 commits into from
Feb 6, 2025

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Feb 5, 2025

Related Issue: #11345

Summary

Adds focusTrapOptions option to allow additional customization of focus-trapping components. It supports the following options from https://github.com/focus-trap/focus-trap#createoptions:

And the following custom prop:

  • extraContainers – allows specifying extra elements (nodes or selectors) to focus trap (e.g., anything appending to the body, etc) when creating/activating the trap. Note: if specified, elements must exist when the focus trap is activated, if extra containers are created afterwards, users can use updateFocusTrapElements(extraContainers)

This also enhances updatesFocusTrapElements() to accept extra containers to allow in the focus trap if these are created after the trap is activated.

Notes

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Feb 5, 2025
@jcfranco jcfranco marked this pull request as draft February 5, 2025 07:20
@jcfranco jcfranco requested review from driskull and Elijbet February 5, 2025 07:20
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Looks good! We'll need to do this for Modal, Popover, and Sheet as well.

@@ -161,6 +161,9 @@ export class Dialog extends LitElement implements OpenCloseComponent, LoadableCo
*/
@property({ reflect: true }) escapeDisabled = false;

/** Allows special focus-trap configuration. */
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Allows for focus trapping configuration?

Copy link
Member

Choose a reason for hiding this comment

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

We'll probably need to document the options as well right? "allowOutsideClick" | "initialFocus" | "returnFocusOnDeactivate"

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally. @geospatialem @DitwanP Any ideas on how we should doc items from another lib? I don't think we call out using focus-trap or other 3rd-party lib anywhere in the doc site.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we don't want to call out focus-trap specifically so we can say "focus trapping"

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

Specifies custom focus trap configuration on the component, where `"allowOutsideClick`" allows outside clicks, `"initialFocus"` enables initial focus, and `"returnFocusOnDeactivate"` returns focus when not active.

I'm not 100% sure if that's the true intended behavior for the options, but perhaps something to that effect?

@jcfranco
Copy link
Member Author

jcfranco commented Feb 5, 2025

Looks good! We'll need to do this for Modal, Popover, and Sheet as well.

Got these PRs standing by:

@jcfranco jcfranco changed the title feat(dialog): add options prop to customize focus-trap feat(dialog, modal, popover, sheet): add options prop to customize focus-trap Feb 6, 2025
@jcfranco jcfranco marked this pull request as ready for review February 6, 2025 00:26
@jcfranco jcfranco requested a review from eriklharper February 6, 2025 00:29
@jcfranco
Copy link
Member Author

jcfranco commented Feb 6, 2025

I'll work on adding automated test coverage for some of these changes and controller-specific testing will be handled in #11305.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

👍

async updateFocusTrapElements(): Promise<void> {
this.focusTrap.updateContainerElements();
async updateFocusTrapElements(
extraContainers?: ExtendedFocusTrapOptions["extraContainers"],
Copy link
Member

Choose a reason for hiding this comment

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

nice, this looks good 👍

@@ -81,7 +71,21 @@ export class Modal
this.updateSizeCssVars();
});

focusTrap: FocusTrap;
focusTrap = useFocusTrap<this>({
Copy link
Contributor

Choose a reason for hiding this comment

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

✨🎮 ✅✨ Nice. Brings all the rest of the components up to date!

* @see https://github.com/focus-trap/focus-trap#createoptions
*/
Pick<FocusTrapOptions, "allowOutsideClick" | "initialFocus" | "returnFocusOnDeactivate"> & {
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this doesn't include escapeDeactivates?

Copy link
Member Author

@jcfranco jcfranco Feb 6, 2025

Choose a reason for hiding this comment

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

I didn't include some options that may conflict with existing component props (e.g., escapeDisabled), but we could revisit this if there's a use case.

* `"initialFocus"` enables initial focus,
* `"returnFocusOnDeactivate"` returns focus when not active, and
* `"extraContainers"` specifies additional focusable elements external to the trap (e.g., 3rd-party components appending elements to the document body).
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this include escapeDeactivates?

@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 6, 2025
@jcfranco
Copy link
Member Author

jcfranco commented Feb 6, 2025

Merging to proceed with testing and verification. LMK if anything needs following up.

@jcfranco jcfranco merged commit 454f8e8 into dev Feb 6, 2025
17 checks passed
@jcfranco jcfranco deleted the jcfranco/11345-add-focus-trap-options branch February 6, 2025 02:57
benelan pushed a commit that referenced this pull request Feb 8, 2025
…cus-trap (#11453)

**Related Issue:** #11345 

## Summary

Adds `focusTrapOptions` option to allow additional customization of
focus-trapping components. It supports the following options from
https://github.com/focus-trap/focus-trap#createoptions:

* `initialFocus` – would replace popover's internal
initialFocusTrapFocus prop
* `allowOutsideClick` – supports
#10682
* `returnFocusOnDeactivate` – supports
#10682

And the following custom prop:

* `extraContainers` – allows specifying extra elements (nodes or
selectors) to focus trap (e.g., anything appending to the body, etc)
when creating/activating the trap. **Note**: if specified, elements must
exist when the focus trap is activated, if extra containers are created
afterwards, users can use `updateFocusTrapElements(extraContainers)`

This also enhances `updatesFocusTrapElements()` to accept extra
containers to allow in the focus trap if these are created after the
trap is activated.

### Notes

* A subset of https://github.com/focus-trap/focus-trap#createoptions
options are exposed as certain configurations might break component
functionality.
* `extraContainers` gets used both when creating and updating the focus
trap target containers
* Tidies up types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants