Clicking another shadowed element inside the focus trap will deactivate the focus trap

[driskull]

When the following settings are set, clicking another shadowed element inside the focus trap will deactivate the focus trap when it shouldn't.

clickOutsideDeactivates: true,
escapeDeactivates: false,

Clicking the "Light Dom Button" was just to verify a normal button doesn't deactivate the focus trap. However, a custom element button with a shadowRoot does deactivate the focus trap.

Example

Example demo patch to see problem

diff --git forkSrcPrefix/docs/js/in-open-shadow-dom.js forkDstPrefix/docs/js/in-open-shadow-dom.js
index 0377780fad82edafdefa7b9cd02f444042a6f267..e3273d9bb82b641cf019bdb1b9e7e9c1a79c445d 100644
--- forkSrcPrefix/docs/js/in-open-shadow-dom.js
+++ forkDstPrefix/docs/js/in-open-shadow-dom.js
@@ -1,5 +1,13 @@
 const { createFocusTrap } = require('../../index');
 module.exports = () => {
+  class CustomButton extends HTMLElement {
+    constructor() {
+      super();
+
+      this.attachShadow({ mode: 'open' }).innerHTML = '<button><slot></slot></button>';
+    }
+  }
+
   class FocusTrapModal extends HTMLElement {
     constructor() {
       super();
@@ -14,6 +22,8 @@ module.exports = () => {
           <a href="#">with</a> <a href="#">some</a> <a href="#">focusable</a> parts.
         </p>
         <p>
+          <custom-button>Shadow Button</custom-button>
+          <button>Light DOM Button</button>
           <button id="deactivate-in-open-shadow-dom" aria-describedby="in-open-shadow-dom-heading">
             deactivate trap
           </button>
@@ -32,7 +42,8 @@ module.exports = () => {
       const focusTrap = createFocusTrap(modalEl, {
         onActivate: () => modalEl.classList.add('is-active'),
         onDeactivate: () => modalEl.classList.remove('is-active'),
-        escapeDeactivates: true,
+        clickOutsideDeactivates: true,
+        escapeDeactivates: false,
       });
 
       document
@@ -45,4 +56,5 @@ module.exports = () => {
   }
 
   customElements.define('focus-trap-modal', FocusTrapModal);
+  customElements.define('custom-button', CustomButton);
 };

example commit: driskull@1be26a4

I tracked the issue down to this function

focus-trap/index.js

Lines 185 to 196 in b781078

	const findContainerIndex = function (element) {
	// NOTE: search `containerGroups` because it's possible a group contains no tabbable
	// nodes, but still contains focusable nodes (e.g. if they all have `tabindex=-1`)
	// and we still need to find the element in there
	return state.containerGroups.findIndex(
	({ container, tabbableNodes }) =>
	container.contains(element) ||
	// fall back to explicit tabbable search which will take into consideration any
	// web components if the `tabbableOptions.getShadowRoot` option was used for
	// the trap, enabling shadow DOM support in tabbable (`Node.contains()` doesn't
	// look inside web components even if open)
	tabbableNodes.find((node) => node === element)

It seems to be having a problem with the slotted <slot> element.

This seems to fix it but I'm not sure if its the right solution...

tabbableNodes.find(function (node) {
  return node === element || node.contains(element);
});

I did notice that even elements that are not "tabbable" in this scenario are deactivating the focus trap. driskull@ade43e5

Let me know how I can help with this issue. :)

[stefcameron]

@driskull Thanks for the detailed report and all your analysis so far! It's very helpful. My first thought is that this is actually an issue with tabbable, not focus-trap, since focus-trap relies on what tabbable finds in each of the containers its given to determine what's in or out of the trap: https://github.com/focus-trap/focus-trap/blob/master/index.js#L380-L400

I'm guessing tabbable isn't finding that other light shadow element you've inserted in the trap for some reason, and since your trap is configured to deactivate on click outside, it deactivates when you click on it (it treats that shadow element as being outside the trap).

If you could dig further into tabbable and see if that's the case, it would be a big help! Pay close attention to getCandidatesIteratively() as that should be getting used by tabbable in this case since you're using shadow DOM elements with focus-trap (assuming, also, you've configured focus-trap's tabbableOptions to activate shadow DOM support, particularly the getShadowRoot option).

[driskull]

@stefcameron I'll dig into the tabbable not finding the button within the CustomButton.

However, it does seem to be an issue with focus-trap as well because even when clicking a custom-element that just has a span inside of it, it deactivates the focus trap. Example: driskull@ade43e5

Seems like focusTrap shouldn't be deactivating in this case.

[driskull]

@stefcameron it actually just seems to be a focus-trap issue.

With the tabbableOptions set as shown below it does have the button in the containerGroups.

tabbableOptions: {
  getShadowRoot: true
},

So it just seems to be a focus-trap issue of deactivating the focus trap when it shouldn't. 90aad49

[stefcameron]

@driskull Thanks for investigating further and for the draft PR. I'll check this out.

[stefcameron]

@driskull So the problem is that we're slotting non-focusable, non-tabbable text nodes into those custom elements you've added, when clicking on either one, it's actually the <slot> element that gets clicked. And since slots are neither focusable nor tabbable, tabbable doesn't find the slot (well, it does, but it looks inside and doesn't count the slot, so to speak). Result being that the slot is seen as being not contained within the focus trap container, and with clickOutsideDeactivates=true configured, the trap deactivates automatically.

If I carefully click on one of the extreme corners of the "Shadow Button" you've added to the demo, the trap remains active because the click is registered on the shadow <button> element, and tabbable has found that one.

If I slot a <button>Shadow Span</button> into your shadow <span> element and click it, the trap also remains active because the click is registered on the <button>, which tabbable has found. Also works if clicking on the button's label, because the click is still registered on the button.

So some additional check is needed in https://github.com/focus-trap/focus-trap/blob/master/index.js#L185-L198 when the element is a <slot>. 🤔

[driskull]

@stefcameron Yep, that's exactly what I was seeing as well.

This did seem to fix it but I'm not sure if its ideal.

tabbableNodes.find((node) => node === element || node.contains(element))

[stefcameron]

@driskull

@stefcameron Yep, that's exactly what I was seeing as well.

This did seem to fix it but I'm not sure if its ideal.

tabbableNodes.find((node) => node === element || node.contains(element))

Good to know we're seeing the same thing. Your suggestion would work, but only as long as there are no nested shadow DOMs since Node.contains() doesn't see into those. If the slot was in a more deeply nested shadow DOM, we'd still miss it.

What we need is tabbable's getCandidatesIteratively() function, but here, and modified to check for containement instead of looking for focusable/tabbable candidates, and only used as a 3rd fallback if the first 2 checks don't work. But I hate to mostly duplicate code, even though the logic of the loop would be different. Might not be a choice, though, since getting tabbable to produce a list of slots or something would be out of its realm of concern.

[stefcameron]

Well, actually your suggestion wouldn't hurt to be added to the second check. But we'd still need the exhaustive 3rd check as a fallback.

[driskull]

Is there no way to get the composedPath of the click event and check to see if the composedPath contains the element in question? That would solve it in all cases.

Something like event.composedPath().includes(focusTrapEl)

[stefcameron]

Is there no way to get the composedPath of the click event and check to see if the composedPath contains the element in question? That would solve it in all cases.

Something like event.composedPath().includes(focusTrapEl)

Interesting. In checkPointerDown(), we call getActualTarget(), which gets uses getComposedPath() to get the actual thing that was clicked, which ends up being the slot.

I think your suggestion would work since that element is effectively a child of that shadow and part of the listed path! Just need to add the typeof event.composedPath === 'function' check in front of it in case the API isn't available.

Is this something you want to add to your PR then? Just want to make sure we don't double-up on the same work.

We'd also need to add a // TEST MANUALLY (Cypress doesn't support Shadow DOM well) comment above line 38 in docs/js/index.js because it really needs to be tested manually (unfortunately, because of Cypress' lack of good support for shadow DOM).

And I noticed that the spanned slot in your PR is missing a closing </slot>

I think that would do it.

[driskull]

@stefcameron updated the PR. Let me know what needs to be changed. Thanks 🍻

[stefcameron]

I appreciate your updates, thank you! Left a quick comment on the PR.

[stefcameron]

@driskull Thanks for your work on that PR. Now that it's merged, I'll publish the fix shortly. 😄

[driskull]

Awesome! Thanks for the help with getting this in

[stefcameron]

Published in focus-trap v7.4.2