[#1729] Fix no trap method with trapStack global#1731
Conversation
Fixes #1729 With #1691, the trap object has a new internal method, `_setSubtreeIsolation()`, but when using the `trapStack` option to allow multiple versions of Focus-trap libraries to run in the same DOM and share the same stack, older versions will not have this method and whenever the newer Focus-trap instance tries to manage the stack, a `preexistingTrap._setSubtreeIsolation is not a function` error will result because it expects the new method to exist on all traps in the stack. Fix this by using optional chaining when calling the new method. That will cause it to be skipped on older traps that don't have it. Of course, this implies that the newer `isolateSubtrees` option cannot be used on older traps, but that should be a given anyway. If you're using multiple Focus-trap libraries in the same DOM, make sure they all support that new feature if you want to use it. Otherwise, results will not be as expected (and not supported).
🦋 Changeset detectedLatest commit: f4ec2f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughGuards calls to a possibly-missing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes (Short, sharp, and slightly less dramatic than the bug it fixes.) 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
index.js (1)
1051-1053: Verify: Optional chaining ontrapitself seems overly defensive.Since
trapis the current instance being created in this same file, and_setSubtreeIsolationis always defined viaObject.definePropertiesat line 1231, the optional chaining here shouldn't be necessary for the current trap.However, I can see this might be for consistency or to prevent issues if someone creates a partial mock/stub in tests. If intentional, a brief comment explaining the reasoning would help future readers understand this isn't just copy-paste.
Not a blocker - just something to consider for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
docs/demo-bundle.js.mapis excluded by!**/*.map
📒 Files selected for processing (3)
.changeset/six-deer-wink.mddocs/demo-bundle.jsindex.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e (firefox)
- GitHub Check: e2e (chrome)
🔇 Additional comments (17)
.changeset/six-deer-wink.md (1)
1-5: LGTM!Changeset is properly formatted with the correct patch bump level. The description clearly explains the fix and links to the issue. 👍
index.js (7)
896-896: LGTM!Optional chaining correctly guards against calling
_setSubtreeIsolationon traps that may not have this method. This location handles thecollectAdjacentElementsfunction where subtree isolation is toggled off before re-collecting elements.
1029-1031: LGTM!This is the key fix location mentioned in the issue. When activating a new trap, we need to disable isolation on the preexisting trap from the stack. If that trap is an older version without
_setSubtreeIsolation, the optional chaining gracefully skips the call. No moreis not a functionexplosions! 💥➡️😌
1066-1075: LGTM!Error recovery path is properly guarded. If activation throws, we attempt to restore the preexisting trap's isolation state - but only if it supports the method. Good catch on this edge case! 🎣
1099-1101: LGTM!Deactivation path correctly guarded. Consistency is maintained across all call sites.
1179-1181: LGTM!Container update path properly protected.
1210-1212: LGTM!Pause state transition correctly handles the optional method call.
1219-1221: LGTM!Unpause transition properly guarded. All the pause/unpause symmetry is preserved with the same defensive pattern.
docs/demo-bundle.js (9)
380-386: LGTM on tabbable library update.Version bump from 6.3.0 to 6.4.0 and the expanded
candidateSelectorpatterns with:not([inert] *)variants look correct. These ensure elements inside inert containers are properly excluded.
1720-1723: LGTM!Transpiled optional chaining in
collectAdjacentElementsmatches the source fix.
1854-1857: LGTM!The preexisting trap guard during activation is correctly transpiled. This is the transpiled equivalent of the key fix at line 1030 in
index.js.
1873-1876: LGTM!Activation finish path correctly transpiled.
1887-1890: LGTM!Error recovery path transpilation is correct.
1911-1914: LGTM!Deactivation path properly transpiled.
1968-1971: LGTM!Container update path transpilation looks good.
1989-2004: LGTM!All pause/unpause state transition guards are correctly transpiled. The pattern is consistent throughout.
1-4: This is a generated file—changes look good.Confirmed:
docs/demo-bundle.jsis generated bynpm run compile:demo(which runs Rollup with Babel transpilation), and it's properly gitignored. The changes correctly reflect the Babel-transpiled version of the optional chaining fix fromindex.js. No manual edits needed here—this will be regenerated during the build process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
index.js (1)
1076-1079: Pattern is solid and mirrors the activation path correctly.The optional chaining properly handles cleanup when activation fails, ensuring older traps can be reverted without errors even if they lack
_setSubtreeIsolation. The defensive coding here matches line 1033 (activation) and follows the same #1729 safeguard.No explicit test coverage for trapStack mixed-version scenarios was found in the codebase, so consider adding a test for this edge case if not already implicitly covered by existing test suites.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
docs/demo-bundle.js.mapis excluded by!**/*.map
📒 Files selected for processing (2)
docs/demo-bundle.jsindex.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e (chrome)
- GitHub Check: e2e (firefox)
🔇 Additional comments (4)
index.js (1)
1030-1033: LGTM! Clean fix for version compatibility.The optional chaining elegantly handles the case where older focus-trap versions on a shared
trapStackdon't have the_setSubtreeIsolationmethod. The inline comment clearly explains the rationale.docs/demo-bundle.js (3)
380-385: LGTM! Tabbable 6.4.0 brings improved inert handling.The updated selectors with
:not([inert]):not([inert] *)properly exclude elements inside inert containers, and the refactoredisInertlogic now prefers the more efficientnode.closest('[inert]')approach. Nice upgrade! 🎯Also applies to: 398-421, 954-954
1853-1857: Transpiled optional chaining looks correct.This is the bundled/transpiled version of the optional chaining from
index.js, properly converted to browser-compatible code. The null/undefined checks before calling the method are functionally equivalent to the?.()operator in the source.
1888-1892: Error recovery path properly transpiled.The transpiled optional chaining in the error recovery block correctly mirrors the source code, ensuring cleanup works even with mixed focus-trap versions.
1 participant
Fixes #1729
With #1691, the trap object has a new internal method,
_setSubtreeIsolation(), but when using thetrapStackoption to allow multiple versions of Focus-trap libraries to run in the same DOM and share the same stack, older versions will not have this method and whenever the newer Focus-trap instance tries to manage the stack, apreexistingTrap._setSubtreeIsolation is not a functionerror will result because it expects the new method to exist on all traps in the stack.Fix this by using optional chaining when calling the new method. That will cause it to be skipped on older traps that don't have it.
Of course, this implies that the newer
isolateSubtreesoption cannot be used on older traps, but that should be a given anyway.If you're using multiple Focus-trap libraries in the same DOM, make sure they all support that new feature if you want to use it. Otherwise, results will not be as expected (and not supported).
PR Checklist
Please leave this checklist in your PR.
npm run demo-bundlein your branch and include the/docs/demo-bundle.jsfile that gets generated in your PR).// TEST MANUALLYcomments here) that can't be fully tested in Cypress have been manually verified.typeof document/window !== 'undefined'before using it in code that gets executed on load.npm run changesetlocally to add one, and follow the prompts).Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.