Conversation
This replaces the previous focus-trap implementation. This allows us to remove the previous implementation that relied on a forked package we are struggling to keep maintained.
|
size-limit report 📦
|
| }; | ||
| private _focusTrapController = new FocusTrapController( | ||
| this, | ||
| () => this.shadowRoot?.querySelector('.dropdown-menu__list') as HTMLElement | null |
There was a problem hiding this comment.
Can these all be replaced with assigned @query objects?
There was a problem hiding this comment.
Yeah, that't probably true, I can take a look at that. I also have gone back and forth if it made more sense to use a "standard" class name like focus-trap for the element we want the focus trap on, instead of targeting specific classes in specific components.
There was a problem hiding this comment.
So, I looked into this, and there is a timing issue. The @query objects don't exist until after first render, so we can't pass them in when the FocusTrapController is initialized.
Since the FocusTrapController is already getting an instance of the element as the first argument, we could assume the class name in the controller, and move the selection there. However, if we ever needed a component with 2 separate areas that had focus traps, we wouldn't have a way to do that. It also makes the coupling (which still exists) a little less explicit.
I think I will update it to accept a selector as the second argument. That feels like a good middle ground. It is a little easier in the components then passing in a getContainer function, but keeps the interface flexible for needing to select more than .focus-trap
Open to thoughts though!
There was a problem hiding this comment.
That sounds good, thanks for the leg work on it!
brentswisher
force-pushed
the
feat/update-focus-trap-library
branch
from
a386e05 to
9c44f6a
Compare
The new focus trap library focuses the first element by default when the trap is activated. This is not ideal for dropdowns, which should only focus the first element if the user has navigated to the dropdown using the keyboard.
2 participants
This change: (check at least one)
Is this a breaking change? (check one)
Is the: (complete all)
What does this change address?
Closes #777
How does this change work?
This replaces the previous focus-trap implementation, with a new focus trap controller that uses a more actively maintained library. It allows us to remove the previous implementation that relied on a forked package we are struggling to keep maintained.
Additional context