Have focusTrap action focused on a specific element using tabIndex

[doic]

Describe the feature in detail (code, mocks, or screenshots encouraged)

After discussing this on discord with @endigo9740 : Let users implement their own tabindex attributes, so they retain full control and encourage best practices By default, focusTrap should default to the first focusable element in the array If specified, focusTrap should target the tabindex supplied by the user (I'll discuss how we pass this below) Update modals/dialogs to accept an optional tabindex, which is passed to the embedded version of focusTrap. Which triggers the above result. Per the interface for providing the tabindex to the focusTrap, we're unfortunately a bit limited due to the current implementation accepting only a single boolean value: <form use:focusTrap={someBoolValue}>

First instinct is to do something like this - however this would be a breaking change and is not possible until the next major release: <form use:focusTrap={{state: someBoolValue, tabindex: 2}}>

But what we'll likely need to do is use a data attribute as a temporary solution. This is optional and non-breaking: <form use:focusTrap={someBoolValue} data-tabindex={2}>

I'm demonstrating tabindex as a number here, but string might be more appropriate.

We should likely keep the existing logic around a default selection (first focusable). This keeps parity with the existing functionality (prevents a breaking change again), as well as provides a nice fallback if users don't specify tabindex values in their content as expected.

What type of pull request would this be?

Enhancement

Provide relevant links or additional information.

No response

[doic]

Limiting the data-tabindex to only form elements may not be the best solution, btw. tabindex attributes can be added to any HTMLElement: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

[doic]

And after reading a bit more, having tabIndexes > 0 is not recommended (even if they use examples > 0 in the doc)

[endigo9740]

Limiting the data-tabindex to only form elements

I saw you mention this on Discord and meant to comment - I never expected this to be limited to only form elements. Any HTML element should allow for tabindex. Though logically, focus should be reserved for commonly focusable elements (anchors, buttons, inputs, etc)

And after reading a bit more, having tabIndexes > 0 is not recommended (even if they use examples > 0 in the doc)

Hmm, that does complicate things a bit. In that case, I'd suggest using data attributes:

<button data-tabindex="0">...</button>
<button data-tabindex="1">...</button>
<div tabindex="0" data-tabindex="2">...</div>

This provides a similar interface, but still gives the focusTrap a specific target that's easily queried.

[doic]

Hmm, that does complicate things a bit. In that case, I'd suggest using data attributes:

<button data-tabindex="0">...</button>
<button data-tabindex="1">...</button>
<div tabindex="0" data-tabindex="2">...</div>

This provides a similar interface, but still gives the focusTrap a specific target that's easily queried.

Wouldn't it be easier to have only one data attribute ? Because if we don't use tabindex, then there is no order to take into consideration, only one element to focus on:

<button>...</button>
<button data-focus="true">...</button>
<div tabindex="0">...</div>

[endigo9740]

I like the simplicity of that, but my concern would obviously be what if the target needs to change? What if a condition is in place that requires you to select element A instead of B.

I'm not sure the specific implementation for ModalSettings yet, but imagine something like this:

function triggerModal(someBool: boolean) {
  const modal: ModalSettings = {
    // ...
    tabindex: someBool ? 0 : 2
  };
  modalStore.trigger(modal);
}

In this case, it might be nice to quickly be able to jump from selecting the data-tabindex="0" or data-tabindex="2" elements based on the conditional statement.

Again, this does require some setup ahead of time by the user, but it provides a simpler implementation here. Seems more flexible, right?

[doic]

To me, it's the same. We can put some logic inside the component template, or in the ModalSettings :-) I'm just concerned about the "tabindex" property in case of ModalSettings. I would rather use this:

function triggerModal(someBool: boolean) {
  const modal: ModalSettings = {
    // ...
    focus: someBool ? id1 : id2
  };
  modalStore.trigger(modal);
}

with id1 and id2 being just elementId. This way we don't mess with tabIndexes > 0 :-)

[doic]

But I prefer the data-focus="true"; I don't see how we can't condition this:

<button>...</button>
<button data-focus="{condition}">...</button>
<div tabindex="0" data-focus="{!condition}">...</div>

[doic]

Another detail, ModalSettings has nothing to do with focusTrap action :-) If we narrow the changes to focusTrap only, it will adapt to other / future utilities that will implement focusTrap !

[endigo9740]

Typically the goal of your template markup is to provide a definition of the visual and non-visual data (read: attributes) for your component HTML. While you certainly can put more of your logic in the template, the questions is if this is the most pragmatic solution. Remember we're building Skeleton for all end users, not only our personal use cases.

In this case I think most folks would prefer the logic be handled in the Javascript, rather than in the template itself. Keep the template slim and "dumb". Let Javascript do it's job.

I'd also encourage you to think beyond a simple boolean example here. Imagine scenarios that have maybe 5-10 items that could dynamically be selected. The requirements for condition within the template grows with your data-focus method, but the upfront requirements are very simple with the data-tabindex approach.

data-focus approach:

<button data-focus={focusedCond === 1}>...</button>
<button data-focus={focusedCond === 2}>...</button>
<button data-focus={focusedCond === 3}>...</button>
<button data-focus={focusedCond === 4}>...</button>
<button data-focus={focusedCond === 5}>...</button>

data-tabindex approach:

<button data-tabindex="0">...</button>
<button data-tabindex="1">...</button>
<button data-tabindex="2">...</button>
<button data-tabindex="3">...</button>
<button data-tabindex="4">...</button>

Another detail, ModalSettings has nothing to do with focusTrap action :-) If we narrow the changes to focusTrap only, it will adapt to other / future utilities that will implement focusTrap !

Correct, the initial implementation is modifying the focusTrap - but the end goal is to allow this new forcusTrap feature to work with modals/drawers/etc. We should not ignore this goal when designing this feature. That would be very short sighted.

[doic]

Makes sense. I'll see how to be smart about this 😁

[doic]

For now, I'm passing ModalSettings.tabIndex to focusTrap, so I had to change:

function focusTrap(node: HTMLElement), enabled: boolean) {}

to

type FocusTrapSettings = { enabled: boolean; tabIndex?: number };
function focusTrap(node: HTMLElement, focusTrapSettings: FocusTrapSettings) {}

I understand it would be a major change and you don't want that, correct ?

[endigo9740]

I understand it would be a major change and you don't want that, correct ?

Correct, any change that's not backwards compatible with the current implementation would be considered "breaking". We cannot provide breaking changes in minor updates to the library. We would have many upset users if we did :)

I would actually be fine if you want to hold off on modifications to the Modal and Drawer right now. Keep it in mind, but let's talk through that next and separately. I'll likely need to dive into the code and refresh how everything is implemented to provide a suggestion for how we should implement. Unfortunately my time is limited today.

Per the changes to the focusTrap, we need to keep:

function focusTrap(node: HTMLElement, enabled: boolean, tabIndex?: number) {}

Then use the node to query for data-tabindex items within it. We do something similar with the Table of Contents action, which scans for header elements:

https://github.com/skeletonlabs/skeleton/blob/master/packages/skeleton/src/lib/utilities/TableOfContents/crawler.ts#L35

Something like:

const focusableElems = node.querySelectorAll('[data-tabindex]);

You then have an array of elements that use this data attribute, and should be able to locate which matches the desired index value - then set focus on that.

[endigo9740]

In the future, before our next major release, we can refactor the focusTrap to our standard args format like so:

interface FocusTrapArgs { enabled: boolean; tabIndex?: number };

function focusTrap(node: HTMLElement, args: FocusTrapArgs) {}

See this pattern used on the ToC crawler as well:

https://github.com/skeletonlabs/skeleton/blob/master/packages/skeleton/src/lib/utilities/TableOfContents/crawler.ts#L6

[doic]

In the future, before our next major release, we can refactor the focusTrap to our standard args format like so:

type FocusTrapArgs= { enabled: boolean; tabIndex?: number };

function focusTrap(node: HTMLElement, args: FocusTrapArgs) {}

Actually, I'm alreay on it with this:

type FocusTrapArgs = boolean | { enabled: boolean; tabIndex?: number };

function focusTrap(node: HTMLElement, args: FocusTrapArgs) {}

I took the idea in the Clipboard action ^^ So it doesn't break anything and nobody will be upset :-)

[endigo9740]

I was considering something like this. I like it. Good call!