focus-trap stuck on elements with positive tabIndex #375
Comments
|
@amje Thanks for reporting this, and for the sandbox repro. I will check this out! |
|
Indeed, this is strange behavior. |
|
I did some investigating and identified the problem in branch |
|
Can reproduce this as well |
|
Thanks for the input. I've got something in the works, but I think it's going to be quite a large change so may take a while for me to get through it. |
Fixes #375 See `// DEBUG` comments in the code, particularly `// DEBUG IDEA` for a possible elegant solution.
|
Slow going, but I just had an idea of how to solve this, so I updated my branch and included my idea in a Will get back to this again later. Unless someone wants to run with this idea and make a PR out of my branch. Just LMK if you're working on it. |
Fixes #375 See `// DEBUG` comments in the code, particularly `// DEBUG IDEA` for a possible elegant solution.
|
@stefcameron I am writing a focus trap and I solved this problem. I can't link to my code because is not on GH yet (the repo is a mess), but I'll leave a snippet in here. Anyway the algorithm is what matter the most. One needs to find the firstZero, lastZero, topTabbable and bottomTabbable in each container. And also create an array with all the positive tab indexes in the trap, ordered by tab index, resolving ties by document order. (I know that focus-trap outsources the search for tabbable elements to focus-trap/tabbable, so you don't have that much freedom of movement here, but still can get the desired results, manipulating the output of focus-trap/tabbable) So then when a tab event occurs if:
If the event target is not an element of the trap, just pretend that its container is the one following it in document order.
Some codeI must admit that it's not that readable, but it doesn't matter, the algorithm is what matters the most, this is just to give an idea of what an implementation would look like. And just to give a bit of context: The array of positive tab indexed is concatenated at the end of And any logic related to radio inputs can be ignored, as focus-trap only handles checked radios. And for the records: there are a few things, like early private assistTabbing = (event: KeyboardEvent) => {
const { target, shiftKey } = event;
if (!(target instanceof HTMLElement || target instanceof SVGElement)) return;
if (this.isUpdateScheduled) this.updateTrap();
this.isUpdateScheduled = false;
const { roots, boundaries, edges } = this.refs;
let rootIndex = roots.findIndex((el) => el.contains(target as Node));
if (rootIndex === -1) {
// Index of first root that follows target, found as the index of the first root that precedes target + 1
rootIndex = (boundaries.findIndex((el) => target.compareDocumentPosition(el) & 2) + 1) % boundaries.length;
}
const firstZero = boundaries[rootIndex * 2];
const lastZero = boundaries[rootIndex * 2 + 1];
if (!firstZero || !lastZero) return;
let destination: Focusable | null = null;
// The logic adopted in this case is the same regardless of `target.tabindex` and is the right logic
// only in case `target.tabIndex < 0` which is always the case when `this.config.lock === true`.
// TODO: make proper logic for `target.tabIndex >= 0`.
if (rootIndex === -1) {
const topTabbable = edges[rootIndex * 2];
const bottomTabbable = edges[rootIndex * 2 + 1];
const precedesTop = target.compareDocumentPosition(topTabbable) & 4;
const followsBottom = target.compareDocumentPosition(bottomTabbable) & 2;
if (precedesTop || followsBottom) {
destination =
edges[(rootIndex * 2 + (shiftKey ? -precedesTop || followsBottom : 2 * followsBottom)) % edges.length];
}
} else if (target.tabIndex === 0) {
if (
(shiftKey && (target === firstZero || areTwoRadiosInSameGroup(target, firstZero))) ||
(!shiftKey && (target === lastZero || areTwoRadiosInSameGroup(target, lastZero)))
) {
destination = boundaries[(rootIndex * 2 - 3 * Number(shiftKey) + 2 + boundaries.length) % boundaries.length];
}
} else if (target.tabIndex > 0) {
const index = boundaries.findIndex((el) => el === target);
destination = boundaries[(index - 2 * Number(shiftKey) + 1 + boundaries.length) % boundaries.length];
} else {
const topmostTabbable = edges[0];
const bottommostTabbable = edges[edges.length - 1];
const precedesTopmost = target.compareDocumentPosition(topmostTabbable) & 4;
const followsBottommost = target.compareDocumentPosition(bottommostTabbable) & 2;
if (precedesTopmost || followsBottommost) destination = edges[shiftKey ? edges.length - 1 : 0];
}
if (destination) {
event.preventDefault();
if (isRadioInput(destination) && !destination.checked) {
getTheCheckedRadio(destination)?.focus();
} else destination.focus();
}
}; // End of assistTabbing();Edit: just noticed that there is a never entered condition (the one with three lines of comment above). There was a time when it made sense cause I was assigning the index of the root for an element outside of the trap to a variable other than As I said, I have a messy repo to take care of and since this fix is not exactly a small one, I don't have any intention to open a PR. Wish you the best with this issue. |
|
@DaviDevMod Thanks for sharing your solution to the problem. Indeed, it's not a trivial task to support elements with positive tab indexes, never mind that altering the tab index is kind of an accessibility faux-pas to begin with. Nonetheless, tabbable does support these elements. I wish there was a simpler solution that leveraged the order already determined by tabbable. Like if tabbing away (i.e. blur) from an element with a positive tab index, find it in the list of tabbable elements discovered by tabbable (since it does order things too, not just search for them and dump them in a list), and then force focus to go to the next element in tabbable's list instead of where focus is wanting to go Although focus-trap typically tries not to get in the way of where focus wants to go other than on container boundaries -- which makes me think that the issue here is probably more that focus-trap is thinking focus is somehow escaping the trap when tabbing away from an element with a positive tab index, and so it's behavior is to bring the focus back into the trap to the most-recently-focused element -- hence why the focus seems "trapped" or "stuck" on that element once it gets focus. Not sure. A few things to investigate still. But I appreciate your code snippet since you highlight some of the challenges. 😄 |
|
Well today I worked a bit on this code and I have to say that you should absolutely not trust the sketchy logic I pointed out for the case the target is not in the trap. With that said, regarding:
You can take the output of tabbable for each container and then find the first/last-zero, top/bottom-tabbable and all the positive tab indexes within that output and push them into arrays. The array of positive tab index would need to be sorted (but in most cases it's empty), while the other two arrays would be already sorted if the containers were sorted. The fact is that when you tab away from a positive tab index, the next positive tab index in the trap could be anywhere. But now that I think about it, it's not possible to get top/bottom tabbables from the array returned by tabbable(), because it's already ordered taking into account tab indexes. tabbable() is an overkill anyway, because you only need to find a few tabbables per container and leave the rest up to the browser, running tabbability checks on all the elements that tabbable queries is unnecessary. But anyway, yes, do your research and weight the options. Edit: regarding this:
I think that positive tab indexes should be treated as boundaries. Any time a tab event is fired from such elements the tabbing can't be left up to the browser. There may be cases in which the browser does what the trap would do, but you still need some logic to handle the rest of the cases and this logic should not be about bringing back the focus, but rather giving it to the right element in the first place. Another edit: actually top/bottom tabbables can be found in the array returned by tabbable(), by comparing the document order of the first tabbable with that one of the first zero tab index and every positive tab index. Though using isTabbable() would be more performant. |
|
@stefcameron I still have a couple of days of housekeeping before I push the repo, but I pushed this branch where you can see a working logic. The culprit of the logic is in If you want to try it, clone the branch The demo is not using I know that implementing such logic implies a lot of changes in focus-trap, but I couldn't find a simpler way to support non-zero tab indexes in my trap. Hope it helps you solve the problem in focus-trap. Edit: if you try the demo and want to add or remove elements, you can do so from |
|
Hello there, apologies for how I exposed my ideas previously, I was having a bad time. There is this function
And returning the element, within the focus trap, that should receive the focus after an Below is the same code made simple ECMAScript, it is self contained and could be copy/pasted as it is, though the first 60 lines are made of tabbable code that should be imported to avoid maintaining it in two places. Code:// String used to query all the candidate focusable elements within the trap.
const candidateSelector =
'a[href], button, input, select, textarea, [tabindex], audio[controls], video[controls], [contenteditable]:not([contenteditable="false"]), details>summary:first-of-type, details';
// <details>, <audio controls> e <video controls> get a default `tabIndex` of -1 in Chrome, yet they are
// still part of the regular tab order. Also browsers do not return `tabIndex` correctly for `contentEditable`
// nodes. In these cases the `tabIndex` is assumed to be 0 if it's not explicitly set to a valid value.
const getConsistentTabIndex = (node) =>
(/^(AUDIO|VIDEO|DETAILS)$/.test(node.tagName) || node.isContentEditable) &&
isNaN(parseInt(node.getAttribute("tabindex"), 10))
? 0
: node.tabIndex;
// Function testing various edge cases. Returns `true` if `candidate` is actually focusable.
function isActuallyFocusable(candidate) {
if (
// If the element has no layout boxes (eg, it has `display: "none"`);
!candidate.getClientRects().length ||
// or is disabled or hidden or an uncheck radio button;
candidate.disabled ||
getComputedStyle(candidate).visibility === "hidden" ||
(candidate instanceof HTMLInputElement &&
(candidate.type === "hidden" || (candidate.type === "radio" && !candidate.checked))) ||
// or a <details> with a <summary> (the summary gets the focus instead of the details);
(candidate.tagName === "DETAILS" &&
Array.prototype.slice.apply(candidate.children).some((child) => child.tagName === "SUMMARY"))
) {
return false;
}
// Elements that are descendant of a closed <details> should not be considered focusable,
// the only exception is the first <summary> of the top-most closed <details>.
const matches = Element.prototype.matches || Element.prototype.webkitMatchesSelector;
const isDirectSummary = matches.call(candidate, "details>summary:first-of-type");
const nodeUnderDetails = isDirectSummary ? candidate.parentElement : candidate;
if (matches.call(nodeUnderDetails, "details:not([open]) *")) {
return false;
}
// Form fields in a disabled <fieldset> are not focusable unless they are
// in the first <legend> element of the top-most disabled <fieldset>.
if (/^(INPUT|BUTTON|SELECT|TEXTAREA)$/.test(candidate.tagName)) {
let parentNode = candidate;
while ((parentNode = parentNode.parentElement)) {
// If `candidate` is nested in a disabled <fieldset>
if (parentNode.tagName === "FIELDSET" && parentNode.disabled) {
for (let i = 0; i < parentNode.children.length; i++) {
// having a <legend> as direct child,
if (parentNode.children.item(i)?.tagName === "LEGEND") {
// If the <fieldset> is not nested in another disabled <fieldset>,
// return whether `candidate` is a descendant of its first <legend>
return matches.call(parentNode, "fieldset[disabled] *")
? false
: parentNode.children.item(i).contains(candidate);
}
}
return false; // The disabled <fieldset> has no <legend>.
}
}
}
return true;
}
const modulo = (number, modulo) => ((number % modulo) + modulo) % modulo;
const candidatesInRoot = (root) => [root, ...root.querySelectorAll(candidateSelector)];
const sortRoots = (a, b) => (a.compareDocumentPosition(b) & 4 ? -1 : 1);
const firstOrLastGenericTabbableInRoot = (root, whichOne = "FIRST", validateTabIndex = (_tabIndex) => true) => {
return (
candidatesInRoot(root)[whichOne === "FIRST" ? "find" : "findLast"](
(el) => validateTabIndex(getConsistentTabIndex(el)) && isActuallyFocusable(el)
) ?? null // Casting `undefined` to `null` for consistency.
);
};
const topOrBottomTabbableInRoot = (root, whichOne = "TOP") => {
return firstOrLastGenericTabbableInRoot(root, whichOne === "TOP" ? "FIRST" : "LAST", (tabIndex) => tabIndex >= 0);
};
const firstOrLastZeroTabbableInRoot = (root, whichOne = "FIRST") => {
return firstOrLastGenericTabbableInRoot(root, whichOne, (tabIndex) => tabIndex === 0);
};
const firstOrLastZeroTabbable = (roots, whichOne = "FIRST") => {
let firstOrLastZero = null;
roots[whichOne === "FIRST" ? "find" : "findLast"](
(root) => (firstOrLastZero = firstOrLastZeroTabbableInRoot(root, whichOne))
);
return firstOrLastZero;
};
const positiveTabbables = (roots) => {
return roots
.map(candidatesInRoot)
.reduce((prev, curr) => prev.concat(curr.filter((el) => el.tabIndex > 0)), [])
.sort((a, b) => a.tabIndex - b.tabIndex);
};
const nextTopOrBottomTabbable = (roots, origin, direction) => {
const originRootIndex = roots.findIndex((root) => root.contains(origin));
// Root from which to start searching for a destination.
let destinationRootIndex = originRootIndex;
// If `origin` belongs to the trap.
if (originRootIndex >= 0) {
// Note that since looking for top/bottom tabbables makes sense only if `origin` has a negative tab index
// (and therefore is untabbable), `origin` can't itself be a top nor a bottom tabbable.
const bottom = topOrBottomTabbableInRoot(roots[originRootIndex], "BOTTOM");
// If the root containing `origin` doesn't have any tabbable elements,
// or if `origin` follows `bottom`, start the search from the succeeding root;
if (!bottom || bottom.compareDocumentPosition(origin) & 4) destinationRootIndex++;
// else if `origin` is in between `top` and `bottom`, leave the focus handling up to the browser.
else if (topOrBottomTabbableInRoot(roots[originRootIndex]).compareDocumentPosition(origin) & 4) return;
} else {
// If `origin` doesn't belong to the trap, start the search from the first root that follows it.
destinationRootIndex = roots.findIndex((root) => origin.compareDocumentPosition(root) & 4);
if (destinationRootIndex === -1) destinationRootIndex = roots.length;
}
// In any case, if tabbing 'BACKWARD' start the search from the preceding root.
if (direction === "BACKWARD") destinationRootIndex--;
for (let i = destinationRootIndex; Math.abs(i) < 2 * roots.length; i += direction === "FORWARD" ? 1 : -1) {
const topOrBottom = topOrBottomTabbableInRoot(
roots[modulo(i, roots.length)],
direction === "FORWARD" ? "TOP" : "BOTTOM"
);
if (topOrBottom) return topOrBottom;
}
throw new Error("There are no tabbable elements in the focus trap.");
};
const nextFirstOrLastZeroOrPositiveTabbable = (roots, origin, direction) => {
const originRootIndex = roots.findIndex((root) => root.contains(origin));
// Root from which to start searching for a destination.
let destinationRootIndex = originRootIndex;
if (originRootIndex >= 0) {
if (direction === "FORWARD") {
if (origin === firstOrLastZeroTabbableInRoot(roots[originRootIndex], "LAST")) destinationRootIndex++;
else return;
} else if (origin === firstOrLastZeroTabbableInRoot(roots[originRootIndex])) {
destinationRootIndex--;
} else {
return;
}
} else {
// If `origin` doesn't belong to the trap, start the search from the first root that follows it.
destinationRootIndex = roots.findIndex((root) => origin.compareDocumentPosition(root) & 4);
if (destinationRootIndex === -1) destinationRootIndex = roots.length;
// If tabbing 'BACKWARD' start the search from the preceding root.
if (direction === "BACKWARD") destinationRootIndex--;
}
const firstOrLastZeroInTrap = firstOrLastZeroTabbable(roots, direction === "FORWARD" ? "LAST" : "FIRST");
for (let i = destinationRootIndex; Math.abs(i) < 2 * roots.length; i += direction === "FORWARD" ? 1 : -1) {
const alternativeDestinationRootIndex = modulo(i, roots.length);
if (
!firstOrLastZeroInTrap ||
origin === firstOrLastZeroInTrap ||
origin.compareDocumentPosition(firstOrLastZeroInTrap) & (direction === "FORWARD" ? 2 : 4)
) {
const positives = positiveTabbables(roots);
if (positives.length) {
return positives[direction === "FORWARD" ? 0 : positives.length - 1];
}
}
const firstOrLastZeroInDestinationRoot = firstOrLastZeroTabbableInRoot(
roots[alternativeDestinationRootIndex],
direction === "FORWARD" ? "FIRST" : "LAST"
);
if (firstOrLastZeroInDestinationRoot) return firstOrLastZeroInDestinationRoot;
}
throw new Error("There are no tabbable elements in the focus trap.");
};
const nextPositiveOrVeryFirstOrVeryLastTabbable = (roots, origin, direction) => {
const positives = positiveTabbables(roots);
let index = positives.findIndex((el) => el === origin);
if (index === -1) {
positives.push(origin);
positives.sort((a, b) =>
a.tabIndex === b.tabIndex ? (a.compareDocumentPosition(b) & 4 ? -1 : 1) : a.tabIndex - b.tabIndex
);
index = positives.findIndex((el) => el === origin);
}
const nextPositive = positives[index + (direction === "FORWARD" ? 1 : -1)];
if (nextPositive) return nextPositive;
const firstOrLastZeroInTrap = firstOrLastZeroTabbable(roots, direction === "FORWARD" ? "FIRST" : "LAST");
if (firstOrLastZeroInTrap) return firstOrLastZeroInTrap;
throw new Error("There are no tabbable elements in the focus trap.");
};
const getDestination = (roots, origin, direction) => {
const originTabIndex = getConsistentTabIndex(origin);
const sortedRoots = roots.sort(sortRoots);
if (originTabIndex < 0) return nextTopOrBottomTabbable(sortedRoots, origin, direction);
if (originTabIndex === 0) return nextFirstOrLastZeroOrPositiveTabbable(sortedRoots, origin, direction);
return nextPositiveOrVeryFirstOrVeryLastTabbable(sortedRoots, origin, direction);
};If you care, here's a brief summary of the logic. Let me know if you are interested in a PR. PS: EDIT: actually the code needed from tabbable is already being exported, as |
Fixes #375 ALL existing cypress tests are passing. TODO: - [ ] get rid of DEBUG comments - [ ] will need to share logic from tabbable about determining tabindex somehow - [ ] check manually tested examples - [ ] see if a cypress test can be added for the new positive-tabindex example - [ ] add changeset
Fixes #375 ALL existing cypress tests are passing. TODO: - [ ] get rid of DEBUG comments - [ ] will need to share logic from tabbable about determining tabindex somehow - [ ] check manually tested examples - [ ] see if a cypress test can be added for the new positive-tabindex example - [ ] add changeset
|
@DaviDevMod Thanks for your update. There seemed to be quite a number of changes in your approach still, or I couldn't make them out with the code. I had long thought this might be solvable by checking for focus escape, which is what strangely happens when a trap has an element with a positive tabindex and the user tabs from it, or somehow a non-positive tabindex element is focused and reverse tabbing, which should go to a positive tabindex element, causes a focus escape. You're right, this is marginally useful, barely upvoted over the past 2 years (!), and not a good a11y practice. Nonetheless, tabbable does support nodes with positive tabindexes and there are probably a lot of forks out there by now that did something to support this. So I finally had a go at it. The only significant change is a whole bunch of new code in Here's my draft PR that still needs polishing and the tabbable work: #974 What do you think? |
|
@stefcameron Nice to see you will work on the fix! The approach of bringing focus back only after it has escaped the trap is fine, but which element will receive the focus? I only gave a quick look at your draft and I don't know exactly what's the algorithm used to find the right destination, however I did add your code to the setup I use to test my focus trap and the current logic doesn't pass the tests. Since the logic to implement is very tricky I suggest you to write some tests before going forward, otherwise you'll lose your mind breaking an edge case while fixing another. In the meantime if you want to try something with the tests in my setup, I created a branch with your code in the repo of my package.This is the branch: https://github.com/DaviDevMod/focus-trap/tree/stef-draft As explained here You need to Then you can run and select the If you want to manual test, you can visit http://localhost:3000/ after running You'll have a trap active for the groups 2 and 4 (the same used in the tests). The tests curretly fail when pressing If you are unsure of what the right tab cycle should be, you can open another tab with my trap on the same elements (using the inputs on the right). Your code is found here: https://github.com/DaviDevMod/focus-trap/blob/stef-draft/apps/demo/src/focus-trap/index.js And imported here when running the dev server: https://github.com/DaviDevMod/focus-trap/blob/stef-draft/apps/demo/src/components/playground/Playground.tsx Or here when running the tests: https://github.com/DaviDevMod/focus-trap/blob/stef-draft/apps/demo/src/components/e2e-playground/E2ePlayground.tsx I'll have a closer look at your draft (and focus-trap logic in general) when possible. |
|
@DaviDevMod Thanks for checking it out briefly. I forgot I had recorded a video of the working solution. I've added it to the PR's description. The description also has a list of still "todo" items, including more testing. This is my preliminary foot forward, so to speak. The logic is simple, really: Rely on what tabbable already determines is the tab order in a given container when executing If the most recently focused node before focus escape was on the edge of a container, then re-use the logic in If there's no suitable node found after all that, then use the existing fallback solution of re-focusing the most recently focused node or the configured initial focused node. Please do have a look when you have a moment. |
|
Relying on The bulletproof way to handle positive tab indexes is to have a sorted array of all the elements with a positive tab index in the trap, find Also in case the focus escapes from an I think the approach you are going for is a good example of the Pareto principle, taking care of most of the cases with little effort, with the added bonus that non-zero tab indexes may be rare enough that "most of the cases" is actually "every real life scenario". |
|
@DaviDevMod I appreciate your thoughts on this, thank you! 😄
This is essentially what I was going for: Least amount of changes for biggest impact for a feature that should really not be used because of the a11y antipattern. Maybe the better thing to do is to close the issue, and just "not go there" because it could open a can of worms I don't really want to support -- that being full, true replication of native browser behavior across a subset of the DOM that happens to be trapped. Still mulling over what's best. I think focus-trap should continue to compliment the browser, not replace it, so I should probably close the issue. But if tons of forks were created (or potential consumers discounting the library) in frustration because of this, then probably good to proceed.
You are bringing up the holes in my approach when it comes to multi-container support. More code will be required to make that work. First thought is to combine all 😬 I'm now tempted to somehow limit positive tabindex support to a single container trap. Something like, whenever If positive tabindexes are rare, hopefully positive tabindexes in multi-container traps are extinct... 😉
Thanks for pointing this out. That's an edge case that might be fairly easy to handle in my draft code since I handle other edge cases I discovered, all based on the MRU focused node. |
I agree. And I personally think that you should give in to the temptation of providing bulletproof support for positive tab indexes only in single-container traps. |
|
@stefcameron Another option worth considering is to solve the issue upstream by allowing Establishing the proper order of tabbable elements across multiple containers should be easier to do upstream, thought this means introducing novel logic there. |
|
@stefcameron I may be wrong, but solving the issue upstream may be as easy as that: focus-trap/tabbable@master...DaviDevMod:tabbable:multi-container Meaning no novel logic at all. |
|
@DaviDevMod This sounds like a great idea! Makes a lot of sense to push the multi-container support upstream to tabbable. I'll check this out further ASAP. |
|
@DaviDevMod I think you're approach is a good one! I made it into a PR which I merged into a new |
|
@stefcameron Glad to see you liked the approach! I just realised, while writing this comment, that it is also necessary to remove nested or duplicate For reference, here is how I did it in my trap: const isNotNestedRoot = (root: Focusable, _index: number, roots: Focusable[]) => {
const isNotNested = roots.every((anotherRoot) => !anotherRoot.contains(root) || anotherRoot === root);
if (!isNotNested) console.warn(`${root} is contained by another root.`);
return isNotNested;
};
const dedupeRoots = (roots: Focusable[]) => {
const dedupedRoots = Array.from(new Set(roots));
if (dedupedRoots.length !== roots.length) {
console.warn('Duplicate elements were found in the "roots" array. They have been deduplicated.');
}
return dedupedRoots;
};
// And later on, something like...
dedupeRoots(resolvedRoots).filter(isNotNestedRoot).sort(byDocumentOrder)
// Where "root" here is the equivalent of "container" in focus-trap.EDIT: that |
|
@DaviDevMod Thanks for the PR. I'm just requesting one tiny tweak, if you don't mind. And good point about testing the containers for dups and nesting. I'll have to add that when I get back to this. |
|
I think another thing with the de-dup and contains thing I'll have to watch out for is that This is starting to get non-trivial... 😬 |
I see, but it's the shadow DOM realm that complicates things, so as a last resort I think it may be acceptable to document that multiple containers are supported only for the regular DOM. But it's not time to throw in the towel just yet, as this particalar
Actually that approach would fail if the nested container precedes its parent in the array of containers and I just found out that you can't rely on compareDocumentPosition when it comes to shadow DOM. This const isNotNestedShadowElement = (a, b) => {
const parent = a.parentNode;
if (parent instanceof ShadowRoot || parent === null) return true;
if (parent === b) return false;
return isNotNestedShadowElement(parent, b);
};
const isNotNestedContainer = (container, _index, containers) => {
const isNotNested = containers.every(
(anotherContainer) =>
container === anotherContainer ||
(container.getRootNode() instanceof ShadowRoot
? isNotNestedShadowElement(container, anotherContainer)
: !anotherContainer.contains(container))
);
if (!isNotNested)
console.warn(`${container} is contained by another container.`);
return isNotNested;
};But there is still the problem of sorting by document order. I think that in the long run, it may be better to bundle shadom DOM utilities in a separate library that would then be imported, removing a lot of complexity from tabbable's codebase. |
I only had a bit of time to dedicate to this today, but I at least gave the whole thing a bit more thought. Since:
If tabbable starts to resort containers, it will probably break expectations/behavior in focus-trap. There's a now long-standing implication that when multiple containers are given, they're given in the order in which focus should move between them, not a random order for focus-trap (and now tabbable) to sort out make "proper". And I'm currently favoring a solution to the nesting thing that avoids checking for nesting by checking for duplicates afterward. I thought accumulating into a But using a plain Array, it might be fairly trivial to eliminate duplicate scopes by finding them all at the top level and eliminating duplicate objects by comparing So just loop through all containers in the order they're given, reducing the result to a single array. Then write some function to de-dup the array exhaustively. Could be like Another thought is since focus-trap uses the containers order verbatim, we might NOT want to |
It's not necessary as discussed here focus-trap/focus-trap#375 (comment): If tabbable starts to resort containers, it will probably break expectations/behavior in focus-trap. There's a now long-standing implication that when multiple containers are given, they're given in the order in which focus should move between them, not a random order for focus-trap (and now tabbable) to sort out make "proper".
From a certain point of view, this is a never reported bug 😆 With sorting out of the way, a simple Regarding reducing I've sent you a PR. |
Agreed, depending on point of view. I smell yet another focus-trap option at some point in the future... 🤪
If we do that, though, isn't there a chance of actually changing the order resulting from But now, in light of the container order "feature", I'm wondering if it would be better to have Downstream in focus-trap, it's no longer necessary to call But this is probably going full circle and not solving the original problem of, "what is the overall positive tab order among all containers". But then, if container order is fixed according to the order in which they're given, that probably means the positive tab order is actually artificially altered. Now focus-trap will have to interject and set a completely different tab order, and that goes against focus-trap getting out of the way and letting the browser do its thing as much as possible, not to mention diverging from reality. Which brings me back to: Positive tabindex elements are only supported in a single-container trap, even if we pursue the multi-container support on the |
If you are referring to this concern:
I was confused 🤣What I should have said is that the approach of deduping the queried candidates works, period. The only reason why it would have failed is that the sorting part was broken, so the whole sorting+deduping would have failed to deliver the expected results.Sometimes I just write down my thoughts as they come, before having formed a clear picture of them. I'm learning to hold my fingers. Deduping before or after sorting should not make any difference. The positive tab order will be the one dictated by the sorting algorithm, which currently is the one implemented in It would also be just fine to limit positive tab index support to single-container traps, and at this point there would be no need for tabbable to support multiple containers. |
|
I really appreciate all your help/input here and on The implications of the multi-container traps in focus-trap plus the shadow DOM stuff, and the fact that positive tab indexes should hopefully be something not widely used, and hopefully even less so across disjoint/random containers, and even less so with one in a shadow DOM and another not... It'll probably be fine, and will keep things much simpler. |
|
@stefcameron You'r welcome! It has been a stimulating discussion. I totally agree with your arguments. Though in the meantime I decided to learn about the Shadow DOM and I started to write some code to seamless sort elements (shadow or not). I'll send you a draft PR when I'm done. |
|
@DaviDevMod I let this simmer for a bit and I've decided I'm going to drop the multi-container support in Tabbable, pursuing positive tab index support for single-container traps only in focus-trap for these reasons (which we've discussed; just restating here for posterity):
So I will pursue #974 by adding a check for positive |
|
@stefcameron totally agree with your decision. I know this issue was about positive tab indexes, but in the discussion it came out negative tab indexes aren't properly handled either. |
👍
By this, do you mean the edge case where the most recently-focused node had a negative tabindex, in which case the browser sets focus on the next element in DOM order, not If so, I have that as a TODO item on the PR, though I was kind of thinking of punting it as a rare edge case in light of everything, but it might not be too difficult to support it. I'm happy to investigate. I know you've demonstrated this behavior earlier in our discussions here. If it's something else, I'm all ears... |
|
@stefcameron Yes, I mean just that edge case. But yeah sorry, even though there would be no need for tabbable with many-containers support, there would be need to compare the document position of the elements returned by It's a shame that there is so little API available to work with the Shadow DOM, I've just learnt how useful it can be. |
I guess I naively thought, given the restriction to a single-container trap and that So I think it might be another stated caveat of positive
I agree! Shadow DOM is powerful in its ability to shield web components for unintended manipulation (be it programmatic- or style-based), but it makes things considerably more difficult to deal with. It's tempting for creating design system components, for example, where it would be easy to ensure components don't get internally styled, changing their appearance in unintended ways counter to design guidelines, but my impression on the outset is they would end-up being more of a pain to deal with than the "in good faith" trust that consumers won't go styling things they shouldn't in the Light DOM... |
|
@stefcameron An alternative to comparing the document position of the elements returned by For example this returned object Line 299 in 8aae81d may have topTabbableNode: focusableNodes.find((el) => isTabbable(el)),
bottomTabbableNode: focusableNodes.findLast((el) => isTabbable(el)),to be used in place of Going a bit off topicI noticed that (sometimes) the library gives up finding the correct container when the focus is about to be given to an element outside of the trap: Line 429 in 8aae81d And I wasn't surprised because handling this case properly requires comparing the document position of elements, which means again a lot of added complexity because of the Shadow DOM. It looks like missing a compareDocumentPosition for Shadow DOM is a recurring problem. You may want to implement it once and for all. |
Wouldn't we also have to add a check for a zero topTabbableNode: focusableNodes.find((el) => isTabbable(el) && getTagIndex(el) === 0) || tabbableNodes[0],
bottomTabbableNode: focusableNodes.findLast((el) => isTabbable(el) && getTabIndex(el) === 0) || tabbableNodes[tabbableNodes.length - 1], |
No, if top/bottom tabbables end up having a positive tab index it's fine. |
Fixes #375 ALL existing cypress tests are passing. TODO: - [ ] get rid of DEBUG comments - [ ] will need to share logic from tabbable about determining tabindex somehow - [ ] check manually tested examples - [ ] see if a cypress test can be added for the new positive-tabindex example - [ ] add changeset
|
@DaviDevMod Alright, PR is updated, including the little demo recording, in which I added the negative tabindex edge case. Please LMK if it all looks good to you, and I think we can finally put this one to bed. 🙂 |
…r traps (#974) * DRAFT: Support elements with positive tabindex attributes Fixes #375 ALL existing cypress tests are passing. TODO: - [ ] get rid of DEBUG comments - [ ] will need to share logic from tabbable about determining tabindex somehow - [ ] check manually tested examples - [ ] see if a cypress test can be added for the new positive-tabindex example - [ ] add changeset * Address remaining TODO items - focus-trap limited to a single container if at least one positive tabindex node is found in any of the containers given to it; an exception is thrown - handled negative tabindex edge case, setting focus to the next tabbable node in DOM order (should be document position, but that would require extensive work not worth the effort for this feature) - Removed all DEBUG comments - Using new tababble `getTabIndex()` API from tabbable v6.2.0 - Added Cypress test - Added Changeset - Manually checked "manual-check" examples * Positive tabindex (#987) * Fix `destinationNode` in case the `target` of the keyboard event has a negative tab index * Refactor `findNextNavNode` Simplified the logic a bit and introduced the following condition: ```js containerGroup.focusableNodes.indexOf(target) >= containerGroup.focusableNodes.indexOf( containerGroup.lastTabbableNode ) ``` checking that `target` is either the `lastTabbableNode` in the container or a focusable (and not tabbable) node preceding it in document order. * Simplify `nextTabbableNode` * Fix `nextTabbableNode` logic for case in which `node` is not tabbable * Cache index of `node` within the `focusableNodes` array, in the body of `nextTabbleNode` * Further simplify `nextTabbableNode` The logic for the case in which `node` has a negative tab index would work also for non-negative tab indexes. Distinguishing between the two cases may be more performant in principle, but not enough to justify any added complexity (at least in my opinion). * Distinguish (again) between `node` with negative and non-negative tab index, in `nextTabbableNode` This reverts commits 075b58c and f198c2b * Revert "Refactor `findNextNavNode`" This reverts commit 91da5ca. * Emphasize j and k keys for custom tab keys example; update demo bundle --------- Co-authored-by: DaviDevMod <98312056+DaviDevMod@users.noreply.github.com>
Hi!
It seems elements with positive tabIndex break tab/shift+tab navigation. When such elements receive focus moving forward become unavailable. While moving backward returning to that element is also unavailable, focus cycle inside container is not working.
Here's a repro example - https://codesandbox.io/s/focus-trap-tab-index-u1fxu
Current behavior
Expected behavior
The text was updated successfully, but these errors were encountered: