modal btn init outline focus style

[aaroncrockett]

Linked Issue

#1591

Description

Current Production Behavior:

• In a modal with a single button:
  • document.activeElement correctly points to the button.
  • Chrome: The button is focused but lacks focus styles. I am unclear if this is expected as it wasn't mentioned.
  • Firefox: Similar to Chrome, but focus styles are also missing even if the user tabs, as noted.

I am guessing the cause may be to be related to :focus-visible, which is not applied until the user interacts with the modal via tab or click. In Firefox, focus styles are never applied when there's just one focusable element. In Chrome, focus styles appear after tabbing to the next element.

Changes in PR:

• Added .btn:focus:not(:focus-visible) to prevent focus styles in undesired situations, such as clicking a button.
• Note: This also changes Chrome's behavior; buttons will now display a blue focus outline when the modal initially opens.

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: 4b6fcf1

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

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/tw-plugin	Patch

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	Oct 22, 2023 7:29pm

[endigo9740]

@aaronmichaelhappe you're raising some great questions in regards to focus styles in your post above. I do believe this is an area that Skeleton could use a bit more attention. This may be a larger conversation than I'm prepare to dive into today though. Let's try to circle back sometime next week - if you're on Discord feel free to ping me there (username: chris) and we can talk directly.

[aaroncrockett]

@endigo9740 sounds good. Yes I will be available Tue or Wed and on - ill ping you around then.

[endigo9740]

@aaroncrockett I've finally had a chance to sit down and review this PR in full. I like the overall goal - I do agree there should likely be some visualization when something is focused. That aligns well with the ARIA APG guidelines:

When a dialog opens, focus moves to an element contained in the dialog. Generally, focus is initially set on the first focusable element. However, the most appropriate focus placement will depend on the nature and size of the content. Examples include:

Source: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/

We tend to favor "first focusable" given that this is very simple and predictable. But this does mean there are a few issues with your styles:

1. Your styles are specific to the .btn class
2. Your styles are specific to children of .modal-footer
3. This only supports .btn elements - while there are plenty of others (anchors, button, inputs, lists, etc)

Remember we support users generating their own custom modals. What if the button lives outside of .modal-footer? What if .modal-footer isn't implement in a custom modal? What if the aren't using the .btn class? What if a button isn't present at all? (though that should be extremely rare). Etc, etc.

I don't have solutions for all these issues, but I wanted to bring that into the conversation here.

Additionally, this is sort of a side tangent, but we've run into issues with this approach to accessibility - where a default item is visually highlighted on open. We've had a surprising number of folks report this behavior as a bug for both drawers and popups. After explaining the -why- folks tend to get it, but initially there's a bit of a disconnect in terms of expectations. Perhaps we add a small blurb in our docs explaining why this behaves in this manner?

[aaroncrockett]

Here are some thoughts I have thus far:
Option 1 - In modal.css. However, could potentially need functionality in Drawer or other future components.

	/* note: inputs, textarea, select and input-group already have focus states from forms.css which seems to be functioning well for first element focus already.  I grabbed the list of elements from focusTrap whitelist. I am still testing a[href] and details.   */
	.modal button:focus:not(:focus-visible),
	.modal a[href]:focus:not(:focus-visible),
	.modal details:focus:not(:focus-visible),
	.modal [tabindex]:not([tabindex='-1']):focus:not(:focus-visible) {
		
		outline-style: auto;
		@apply outline-[-webkit-focus-ring-color];
	}
}
	/* :focus:not(:focus-visible) avoids styling outline in undesirable situations and puts focus on first element when a modal is opening. */

Option 2 - Since the issue is related to focusTrap, what about adding a class on the parent element (ie, a focus-trap class)? Or a similar approach? Similar to how in Table component, a class is added within actions.ts. Styles are applied within plugin/styles.

// Table/actions.ts
	const classAsc = 'table-sort-asc';
	const classDsc = 'table-sort-dsc';

// components/tables.css	
	.table-sort-asc {
		@apply after:opacity-50 after:!content-['↑'];
	}

	.table-sort-dsc {
		@apply after:opacity-50 after:!content-['↓'];
	}

Other
Taken from -> https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/
When a dialog closes, focus returns to the element that invoked the dialog unless (and caveats are given)
Currently this functionality isn't in place. Would this be something you would want to explore in another PR?

[endigo9740]

@aaroncrockett, when talking modals specifically there's no need to append a new class, as known a class is already present. The "windowed" region of the modal (canned or custom) is always wrapped in .modal. Given this, you can rely on it being the entry point for the selector. Something like:

.modal *:focus:not([tabindex='-1']) { ... }

I've refined the selector in a few ways, which might provide more desirable results:

1. Focuses purely on the .modal region
2. Uses * to specify "any children within"
3. Uses a pseudo selector to determine if that particular element is focused
4. Excludes elements designated non-focusable via tabIndex

With the general idea to start with the broadest scope, then whittling it down. But not have to specify each type of focusable element that exists.

This seems to be providing the expected results in a REPL. Though I'd advise more testing when implemented:
https://svelte.dev/repl/d07068fda44b4e41b76dfa6de794a36a?version=3.32.3

[endigo9740]

@aaroncrockett Fridays are my typical PR review days. Just checking in on this. I never heard back after my last message above.

[aaroncrockett]

@endigo9740 Thanks for checking in. I should be able to get to this coming week in time for next PR review.

[aaroncrockett]

I did notice a couple issues when testing further:

With outline-style alone, in regards to the first focused element, the blue default ring (applied to other focus elements) doesn't appear in chrome. In firefox, it appears but doubles up on inputs, as inputs are styled in. So I adjusted to add -webkit-focus-ring-color, and also exclude the styled inputs.

Firefox, without excluding inputs:

Chrome without -webkit-focus-ring-color:

Chrome with -

webkit-focus-ring-color:

[endigo9740]

@aaroncrockett Sorry for how drawn out this has been. I'm typically quicker to respond but have been short on time lately! Thank you for your patience!

Just tested the change and so far this seems to be working to my expectations. I expect some folks will have very strong feelings about this, but I feel this is much closer to modal a11y guidelines. Which is the most important thing.

Only two things I would request to finish this off:

1. Add a small comment above the style explaining it's purpose. This introduces a pretty beefy CSS selector, so we want to retain context for it's purpose. Something like "Provides initial focus selection on opening the modal".
2. Move the PR from Draft -> Ready for Review

I'll then be happy to merge. If you can get this in by Tuesday next week, then this can go out alongside our next release.

Thanks again!

[endigo9740]

@aaroncrockett thanks for the change. It completely slipped my mind, but the Changeset is missing. Follow the instructions in the original post above to add this.

• Make sure to target /packages/tw-plugin
• This should be a patch change
• Message can be: chore: Updated modal focus state style or similar

If you don't get it by Monday I'll jump on this. No worries either way!

[aaroncrockett]

Thank @endigo9740 I did think of this but also noticed these instructions "If you modify files in /packages/skeleton" and assumed since the change was in packages/plugins -- it didn't apply to this.

So out of curiosity and for future PRs, what are the conditions under which we run a changesets? I also noticed one of the commands on on the check list was out of date so I'd be glad to update the template as another issue along clarifying when to use changesets, if you would like.

[endigo9740]

@aaroncrockett the template instructions pre-date the introduction of tw-plugin. So the instructions should read, any changes inside /packages. So changes to the core or plugin - get a changeset. Changes to the docs - those dont.