Feat/focus trap specified

[doic]

Linked Issue

Closes #2190

Description

The goal is to allow focusTrap to focus on a specified element. If no element is specified, focusTrap will focus on the first focusable element, taking into consideration the tabindex attributes:
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

Focus specified elements are targeted thanks to their data-tabindex attribute, which don't conflict with the tabindex attribute.

Changsets

Instructions: Changesets automate our changelog. If you modify files in /packages/skeleton, run pnpm changeset in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should be minor while chores and bugfixes should be patch. Please prefix the changeset message with feat:, bugfix: or chore:.

Checklist

Please read and apply all contribution requirements.

• This PR targets the dev branch (NEVER master)
• Documentation reflects all relevant changes
• Branch is prefixed with: docs/, feat/, chore/, bugfix/
• Ensure Svelte and Typescript linting is current - run pnpm check
• Ensure Prettier linting is current - run pnpm format
• All test cases are passing - run pnpm test
• Includes a changeset (if relevant; see above)

[changeset-bot]

🦋 Changeset detected

Latest commit: 0fb8dc4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	Minor

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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Dec 19, 2023 8:41pm

[endigo9740]

Thanks @doic everyone looks good with a quick scan. I tend to do proper PR reviews on Fridays, so expect at least at least some preliminary feedback tomorrow. When you're ready for a proper review, tap the "ready for review" option here. Thanks!

UPDATE

Per the CI deployment, it looks like svelte-check is failing. If you could please review that with pnpm run check please:

[doic]

Done. I had run pnpm check before messing with the Modal, sorry :-(

[endigo9740]

@doic so a few issues here -

1. svelte-check appears to still be in a failing state. Check the UI just below this comment for details.
2. Supplying a tabIndex in drawerSettings or modalSettings seems to work, but only if it's a value greater than 0. Remember Javascript treats the numeric 0 as a falsey state, so check your conditional logic.
3. Supplying tabIndex as a prop does not appear to be working at all for the either the Drawer or Modal components. I'd recommend checking the flow the data to locate where this is failing
4. I have some suggestions per the documentation (both page and JSDoc comments), but we can return to these after the functional issues are sorted.

Per Issue 1, by making the value of data-tabindex a string it might be more familiar to folks and better match the native iteration, ex:tabindex="3". This would likely also fix 0 being treated as a falsey statement.

Per issues 2 and 3 above, this can be tested by inserted focusable elements with data-tabindex values that are presented out of order:

<input type="text" placeholder="test-1" data-tabindex={1} />
<input type="text" placeholder="test-2" data-tabindex={0} />
<input type="text" placeholder="test-3" data-tabindex={2} />

• When you set a prop of tabIndex={0}, I expect it to focus the second item, but instead it defaults to the first
• When you set a prop of tabIndex={2}, I expect it to focus the third item, but instead it defaults to the first
• When you pass tabIndex: 0, I expect it to focus the second item, but instead it defaults to the first
• When you pass tabIndex: 2, it correctly selects the third item

Please @ me here when you've corrected these issues and are ready for a follow up review please!

[endigo9740]

One more quick ask - can you put a space between feat: and the description in the changeset? This is the line item text that will be inserted into our auto-generated changelog. So it'll be provided verbatim, formatting and all:

[doic]

Weird. Check command worked on my machine. I'll check everything tomorrow morning. Thanks for the feedback 👍

[doic]

OK - I had run pnpm run check in packages dir, but not in svelte.dev dir. Makes sense now 😅.
I did choose to work with numbers because it felt more practical for some logic, like:

someBool ? tabIndex : tabIndex + 1

Obviously, this makes tabIndex = 0 falsey, which is the perfect default value.
But it makes impossible to target a data-tabindex="0" 😑
If you think working with string is the best, let's do this.
I didn't take into consideration passing tabIndex to Modal and Drawer components as a prop, I will work on it !

[doic]

By the way, the more I see tabindex and data-tabindex HTML attributes side by side, the more I think using tabIndex as a parameter for this feature is wrong. It can be very confusing.
It's not an index, and it has nothing to do with TAB...
Can we find a better name ? I think "focus + target" sounds good 😁

[endigo9740]

Hey @doic I handle PR reviews on Sundays now. Just gave this a quick scan. I see you received some feedback from Mahmoud, but I don't see any commits beyond that. I'll try to squeeze in another review on Tuesday (our next release) otherwise expect a follow up later in the week.

[doic]

Yes sorry, I've been quite busy these days. I'll try to get back to it during the week.

[doic]

By the way, the more I see tabindex and data-tabindex HTML attributes side by side, the more I think using tabIndex as a parameter for this feature is wrong. It can be very confusing. It's not an index, and it has nothing to do with TAB... Can we find a better name ? I think "focus + target" sounds good 😁

I'm back 😁
Nobody answered this comment - can someone please give some feedback ?
After reading a bit more about other ways to handle this, I found this createOptions.initialFocus idea:
https://www.npmjs.com/package/focus-trap
I'm not talking about the whole implementation, only the name of it.
I find it easier to understand - initialFocus / data-initialfocus instead of tabindex / data-tabindex.

[doic]

The easiest way - imo - to do this would be with css selectors (allowing to use id attributes if we prefer):

<form use:focusTrap={{ enabled: true, initialFocus: 'input[type=email]' }}>
	<input type="text" placeholder="Name" />
	<input type="email" placeholder="Email" />
	<button class="btn variant-filled-primary">Submit</button>
</form>

[endigo9740]

@doic I'll plan to do PR reviews tomorrow. I'll dive into this then.

[endigo9740]

@doic a lot of the original ideas were based on a tabindex-like system, something that would handle fallbacks and conditional statements where the focusable element might change as state updates.

Imagine you had a conditional statement inside a Drawer or Modal content like this:

<div>
    {#if someBool}
        <button data-tabindex="0">Button 1</button >
    {#else}
        <button data-tabindex="1">Button 2</button >
    {/if}
    <button data-tabindex="2">Button 3</button >
</div>

Based on the state of someBool either Button1 or 2 will be the top-most focusable element, but that will differ based on state that may or may not change before this UI is rendered to the screen. With the tabindex-like system we can handle this easily. Including any scope of complexity (multiple conditionals, etc).

However, I do agree the naming is not quite semantic. We're appending attributes purely meant to be used by the focus trap. As such, I'd recommend we do strive to maintain the same goals for functionality, but provide more semantic naming.

I tend to favor shorter, single word values whenever possible. Then let JSDocs provide more context via Intellesense. So I'd recommend something like:

• initialFocus -> focus
• data-initialfocus -> data-focus (or data-focusindex)

Let me know if that makes sense.

[doic]

@endigo9740, in your example, we'll have either 0 or 1 as the smallest available data-focusindex.
So what focus value should we pass to the focusTrap action ?

We currently have something that behaves like this:

But I sense (in your example) that the focus/tabIndex value passed to the focusTrap action is not needed anymore, because the action has to determine what is the smallest data-focusindex value within the element.

If I'm correct, then this feels very tabindex-like and easier to use. We just need to use tabindex and/or data-focusindex and let focusTrap decide which element to focus on.

It will be more consistent with the previous focusTrap signature - no need to pass a boolean | object parameter as I was doing.

[endigo9740]

But I sense (in your example) that the focus/tabIndex value passed to the focusTrap action is not needed anymore, because the action has to determine what is the smallest data-focusindex value within the element.

Yeah sorry, two different thoughts there that bled together. Yeah if we have the index model we go for the smallest index and a specified value is not required.

If I'm correct, then this feels very tabindex-like and easier to use. We just need to use tabindex and/or data-focusindex and let focusTrap decide which element to focus on.

Yep, exactly this.

[doic]

@Mahmoud-zino don't hesitate to give feedback on this last commit, the previous ones were very useful 🙏

[Mahmoud-zino]

@doic the code looks good to me, I would just suggest changing the description in the docs to something like this:

---

Focus order

Specify the order of focusable elements by assigning a value to the data-focusindex attribute. The element with the lowest value will be focused first.

[doic]

@Mahmoud-zino done 🙏

[endigo9740]

@doic I know this issue is waiting on me, but my schedule has blown up lately. I have some free time coming up in a couple weeks, so worst case I'll jump on this again then. Thanks so much for your patience, I'm very excited to get this implemented when possible!

[doic]

@endigo9740 sure, no problem :-) I hope this will make it to one of the next releases !

[endigo9740]

@doic I'm actually doing PR reviews as we speak today! So far we're down from 14 -> 5. So I'm slowly inching my way in this direction :)

[endigo9740]

@doic with the smaller PRs sorted today I'm going to aim to jump on yours and the few other remaining tomorrow post-release. Your PR is a bit more broad in scope so I want to make sure I devote enough time to testing, rather than to rush it in last minute.

Expect some updates on this tomorrow. If things are close enough I may go ahead and make a few modifications myself directly to help wrap this up. I'll let you know!

[endigo9740]

@doic @Mahmoud-zino I've pushed some very minor changes to the documentation info and examples. If you two sign off on this I'm happy to merge. The feature is working exactly as expected. Great job!

[doic]

The description looks better, indeed, but I don't understand why hardcoding use:focusTrap={true} line 104 ?
I basically copied the example above - where use:focusTrap={isFocused} (line 54)

[endigo9740]

In the first example we're teaching folks how to toggle the focus trap on and off. No need to repeat that here. The area of focus is focusindex values.

We strive to keep our examples as minimal as possible, given the small team. It keeps things simpler to maintain over time the less repetition we have.