Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Focus & Modal #229

Open
akiarostami opened this issue Aug 22, 2023 · 5 comments
Open

Focus & Modal #229

akiarostami opened this issue Aug 22, 2023 · 5 comments

Comments

@akiarostami
Copy link

akiarostami commented Aug 22, 2023

First of all, wonderful set of components! Thank you for creating these.

There is an issue with the Modal component. Let's say we have a button that opens a Modal (e.g. «Open ConfirmationModal» button in your Modal documentation page). If the user uses keyboard / tab to move focus to that button, the user then can keep pressing «space» and it would keep opening new Modals.

Modals in general should save the last focused element, and then use a focus trap to prevent users from changing focus. In order to create a Focus Trap, you can simply bracket the dialog node with two invisible, focusable nodes (<div tablindex="0" />). While this dialog is open, we use these to make sure that focus never leaves the document even if dialogNode is the first or last node.

@akiarostami
Copy link
Author

You may find the code below useful:

export let focusAfterClosed = null;

let dialogNode;
let lastFocus;
let ignoresFocusChange;
let focused = null;

function modalAction(node) {
	focused = document.activeElement;
	// mix of https://github.com/KittyGiraudel/focusable-selectors/blob/799829e3b8c329d679b3b414b5dfcfa257a817cf/index.js
	// and https://github.com/focus-trap/tabbable/blob/baa8c3044fe0a8fd8c0826f4a3e284872e1467a5/src/index.js#L1-L13
	const focusableCandidateSelectors =
		'a[href]:not([tabindex^="-"])' +
		',area[href]:not([tabindex^="-"])' +
		',input:not([type="hidden"]):not([type="radio"]):not([disabled]):not([tabindex^="-"])' +
		',input[type="radio"]:not([disabled]):not([tabindex^="-"])' +
		',select:not([disabled]):not([tabindex^="-"])' +
		',textarea:not([disabled]):not([tabindex^="-"])' +
		',button:not([disabled]):not([tabindex^="-"])' +
		',iframe:not([tabindex^="-"])' +
		',audio[controls]:not([tabindex^="-"])' +
		',video[controls]:not([tabindex^="-"])' +
		',[contenteditable]:not([tabindex^="-"])' +
		',[tabindex]:not([tabindex^="-"])' +
		',details>summary:not([tabindex^="-"])' +
		',details:not([tabindex^="-"])';
	// attemptFocus, focusFirstDescendant, focusLastDesendant, trapFocus logic are
	// ported from https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html
	const attemptFocus = (element) => {
		ignoresFocusChange = true;
		try {
			element.focus();
		} catch {
			// ignore
		}
		ignoresFocusChange = false;
		return element === document.activeElement;
	};
	function focusFirstDescendant(element) {
		if (element) {
			const descendants = element.querySelectorAll(focusableCandidateSelectors);
			for (const el of descendants) {
				if (attemptFocus(el)) break;
			}
		}
	}
	function focusLastDescendant(element) {
		if (element) {
			const descendants = element.querySelectorAll(focusableCandidateSelectors);
			for (let i = descendants.length - 1; i >= 0; i--) {
				if (attemptFocus(descendants[i])) break;
			}
		}
	}
	function trapFocus(event) {
		if (ignoresFocusChange) return;

		if (dialogNode.contains(event.target)) {
			lastFocus = event.target;
		} else {
			focusFirstDescendant(dialogNode);
			if (lastFocus === document.activeElement) {
				focusLastDescendant(dialogNode);
			}
			lastFocus = document.activeElement;
		}
	}
	document.addEventListener('focus', trapFocus, true);
	focusFirstDescendant(node);
	return {
		destroy() {
			document.removeEventListener('focus', trapFocus, true);
			if (focusAfterClosed) focusAfterClosed?.focus();
			else focused?.focus();
		}
	};
}

And then, add this at the top and at the bottom of your Modal:

<!-- svelte-ignore a11y-no-noninteractive-tabindex -->
<div tabindex="0" />

@vnphanquang
Copy link
Owner

vnphanquang commented Aug 22, 2023

@akiarostami Thank you for this. Without doubt modal context should trap focus within it. However, there are a couple of reasons why this was not included in @svelte-put/modal:

  • In a past project, i had to implement an overlay that is draggable and still lets users interact with other elements on the page. In this scenario the core implementation is exactly the same as that of modal (it's not a modal and more like a floating window, i know, forgive me), except for the focus trap part.
  • @svelte-put/modal current implementation is mixed between 2 strategies: lib users either extend the predefined Modal component or create their own custom modal. I kinda regret this; I should have pursued the "headless" path more aggressively, as I've recently done with @svelte-put/tooltip & @svelte-put/noti, where lib only provides the component stack logics and leave the UI completely out. My plan is to deprecate the predefined components and simplify the public interface in the next major release. Once that is done, the UI, including focus trap, is up to lib users; and things like clickoutside and movable should be easily added on demand as svelte actions.
  • recently, HTML dialog has gone more and more mainstream. There is in fact a showModal method on HTMLDialogElement that natively does focus trap. I'm experimenting with ways to wrap around dialog and hopefully can take advantage of those native supports.

I'm curious to hear more of your thought my reasoning above. In the meantime, would it be helpful if I introduce a svelte-put/focus-trap svelte action that perhaps will help you with the reported issue here?

@akiarostami
Copy link
Author

@vnphanquang Thanks for taking the time and writing this thoughtful answer.

  • I agree that a draggable overlay can be confusing. In most cases, if we have an overlay, we should not be able to click or activate anything underneath it.As a matter of fact, I can't even think of a scenario that someone would want to do that. So my overall thought is if there's an overlay, then there should be a focus trap. But perhaps I'm missing some use cases?
  • I agree that headless is the right way to go. In my project, I just switched from svelte-sonner to @svelte-put/noti for that reason, even though I really liked svelte-sonner. But I still think focus trap, and clickoutside are not part of the UI, they are part of the functionality underneath. IMHO, a good Headless component should fully cover the UX part, and only leave the cosmetics to the designers. Focustrap, clickoutside, etc. for me fall in the UX area, not UI. I would love to still see these supported in your modal component.
  • Honestly, I have not used HTML Dialog, as it usually takes a long while before all browsers smoothly support features like this, and usually they leave a lot to desire in the user experience area. But I will take a closer look. Thank you for poitning it out.

@vnphanquang
Copy link
Owner

vnphanquang commented Aug 23, 2023

Thanks very much for the follow-up @akiarostami. Very glad to hear that you are finding noti useful and we are in agreement with the headless path.

[...] if there's an overlay, then there should be a focus trap [...]

Yeah, i agree; in that use case it wasn't the best UX, but i didn't have a say in the decision. And honestly it kinda made sense for that particular application at the time.

But the point is still, i don't want to force any opinion upon lib users as i can never predict what this is being used for.


IMHO, a good Headless component should fully cover the UX part, and only leave the cosmetics to the designers.

Same as above, my view is that I should only provide the bare minimum, and an interface that allows features like clickoutside for focustrap to be added easily. For further enhancements and best practices, i prefer to have recipes for lib users to copy codes from. It's easy to add code to lib, but often hard to reduce lib code, yeah?

I'm imaging something like this

Click to see code
<!-- MyCustomModal.svelte -->
<script lang="ts">
	import {
		type ModalInstance,
		// optimized, tree-shakable pieces from counter libs (svelte-put/*)
		escape,
		clickoutside,
		focustrap,
		lockscroll,
	} from '@svelte-put/modal';
	import { movable } from '@svelte-put/movable';
	
	// injected prop by lib
	export let modal: ModalInstance;
</script>

<div class="modal" use:focustrap use:movable use:clickoutside use:escape use:lockscroll>
	<!-- content -->
</div>

code


[...] it usually takes a long while before all browsers smoothly support features like this [...]

Totally. HTML Dialog, however, is now supported in all major browsers. I think it's probably time we adopt it. Lots of good things it does for free, but i'm not entirely sure how extendable it can be. Experimentation is needed.

Also not quite related but I've been impressed by Melt UI and perhaps there is inspiration worth taking from there.

Your point of view is very valuable to the the redesign of this package. Please do provide feedback about the code sample above. Thank you!

@akiarostami
Copy link
Author

You're very kind, @vnphanquang.

Your recipe solution looks very clean.

I've recently seen Melt UI, but I haven't played with it yet. Now that you're recommending it, then I feel I should!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants