♿ Add checks on focusable elements to include target element #3222
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@beltsofplenty been trying a few things, but nothing seems to be working to add the target element yet. |
chuckcarpenter
force-pushed
the
feat/tab-to-target
branch
from
dbcd12d to
2e9e9f3
Compare
There was a problem hiding this comment.
Pull Request Overview
A concise update to include the target element in the focus trap by assigning it a tabindex and extending the modal’s focus-cycling logic.
- Adds
tabindex="0"to non-body targets inStepso they become focusable. - Registers a new
handleFocuslistener in the Svelte component to wrap Tab navigation around the target. - Integrates the target into forward and backward Tab handling in
shepherd-element.svelte.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| shepherd.js/src/step.ts | Adds tabindex="0" for non-document.body targets to include them in focus trap |
| shepherd.js/src/components/shepherd-element.svelte | Registers a focus-trapping keydown listener and updates Tab navigation logic |
| @@ -649,6 +649,10 @@ export class Step extends Evented { | |||
|
|
|||
| target.classList.add(`${this.classPrefix}shepherd-enabled`); | |||
| target.classList.add(`${this.classPrefix}shepherd-target`); | |||
| // adds the target element to the focus trap (if not body), but not for document.body | |||
| if (target !== document.body) { | |||
| target.setAttribute('tabindex', '0'); | |||
There was a problem hiding this comment.
[nitpick] Overwriting any existing tabindex may unintentionally change the element’s tab order. Consider checking for an existing tabindex attribute before setting or using tabindex="-1" if you only need programmatic focus.
| target.setAttribute('tabindex', '0'); | |
| if (!target.hasAttribute('tabindex')) { | |
| target.setAttribute('tabindex', '0'); | |
| } |
Copilot uses AI. Check for mistakes.
| * @private | ||
| */ | ||
| function handleFocus(e) { | ||
| if (e.keyCode === KEY_TAB) { |
There was a problem hiding this comment.
[nitpick] Using keyCode is deprecated and less descriptive. Prefer checking e.key === 'Tab' or e.code === 'Tab' for clarity and future compatibility.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
| if (document.activeElement === lastFocusableElement) { | ||
| e.preventDefault(); | ||
| // Focus target element when tabbing forward from last element | ||
| if (step.target) { |
There was a problem hiding this comment.
I am not sure step.target is what we want here. I believe that is usually document.body. I think we will have to grab the element from attachTo.
|
Closing in favor of #3230 |
Adds target element to focus trap to address #3202