fix: make canClickTarget work when a classPrefix is set - #3481
fix: make canClickTarget work when a classPrefix is set#3481chuckcarpenter wants to merge 1 commit into
Conversation
`Step` adds `shepherd-enabled` and `shepherd-target` to the target with the tour's `classPrefix`, but adds `shepherd-target-click-disabled` unprefixed. The rule that implements click blocking required all three unprefixed on the same element, so with `classPrefix: 'my-tour-'` the target ended up with `shepherd-target-click-disabled my-tour-shepherd-enabled my-tour-shepherd-target`, the selector could not match, and `canClickTarget: false` silently did nothing. Key the rule solely on `shepherd-target-click-disabled`. That class is already unprefixed and is added only when `canClickTarget === false`, so the other two carried no semantic weight — they were only ever incidental. A static stylesheet cannot know the runtime prefix, so dropping them is the fix. The class is repeated three times to hold the selector at its original 0-3-0 specificity, leaving cascade weight unchanged for anyone already overriding it; cssnano preserves the repetition, so the shipped dist/css/shepherd.css keeps 0-3-0 as well. The cypress tests attach to a new fixture in the dummy page whose target and child each carry a competing `pointer-events: auto` declaration — 0-2-0 on the target, 0-1-0 on the child. Without that competition neither half of the rule is actually under test: `pointer-events` is inherited, so a plain child of a blocked target computes `none` even with the `... *` clause deleted, and nothing else on the page contests the specificity. With the fixture, collapsing the repeated class or deleting the descendant clause each turns the tests red. Three caveats worth knowing, since the bug has been latent since 2021: - An app that set `canClickTarget: false` under a `classPrefix` and came to rely on the target staying clickable will now find it blocked. The escape hatch is `canClickTarget: true`, or omitting the option. - Any element that application code manually tagged with `shepherd-target-click-disabled` outside a tour now gets `pointer-events: none`. That class is Shepherd-internal and named for exactly this effect. - A prefixed tour can now trap itself, exactly as an unprefixed one already could. If `stepsContainer` puts the popup inside the target, or an ancestor of the popup (`document.body`, say) is listed in `extraHighlights`, the descendant half of the rule kills the tour's own buttons. Those steps need `canClickTarget: true`. One pre-existing leak is disclosed rather than silently carried. Calling `Step#show()` directly — rather than through `Tour#show`, which hides the current step first — re-resolves `extraHighlights` before tearing the step down, so elements that have since dropped out of the selector keep the classes they were given. Under a prefix that leftover `shepherd-target-click-disabled` used to be inert and is now permanent `pointer-events: none`. The same call leaks `highlightClass` and the prefixed `shepherd-enabled`/`shepherd-target` identically, so it is a `_show` ordering bug that predates this change and wants its own commit, not a widening of a CSS selector fix. The `attachTo` target itself is not affected: `_setupElements` tears down while `step.target` still points at the old element, and only `setupTooltip` reassigns it. landing/src/styles/shepherd.css is a hand-vendored copy of the minified stylesheet that nothing in landing's build regenerates. Its selector is updated here too, so shepherdjs.dev stops shipping a stylesheet that still has the bug. Tours without a `classPrefix` are entirely unaffected: the new selector matches a strict superset of the old one at identical specificity. Fixes #1298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change keeps ChangesTarget click blocking
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This localized fix makes click blocking work for tours that use a classPrefix while preserving existing specificity and behavior for unprefixed tours. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
shepherd.js/test/cypress/integration/element-targeting.cy.js(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.8.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. shepherd.js/test/unit/step.spec.js(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.8.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. 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 |
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on 🛟 Help
|
1 participant
Stepaddsshepherd-enabledandshepherd-targetto the target with thetour's
classPrefix, but addsshepherd-target-click-disabledunprefixed.The rule that implements click blocking required all three unprefixed on the
same element, so with
classPrefix: 'my-tour-'the target ended up withshepherd-target-click-disabled my-tour-shepherd-enabled my-tour-shepherd-target,the selector could not match, and
canClickTarget: falsesilently did nothing.Key the rule solely on
shepherd-target-click-disabled. That class is alreadyunprefixed and is added only when
canClickTarget === false, so the other twocarried no semantic weight — they were only ever incidental. A static
stylesheet cannot know the runtime prefix, so dropping them is the fix. The
class is repeated three times to hold the selector at its original 0-3-0
specificity, leaving cascade weight unchanged for anyone already overriding it;
cssnano preserves the repetition, so the shipped dist/css/shepherd.css keeps
0-3-0 as well.
The cypress tests attach to a new fixture in the dummy page whose target and
child each carry a competing
pointer-events: autodeclaration — 0-2-0 on thetarget, 0-1-0 on the child. Without that competition neither half of the rule
is actually under test:
pointer-eventsis inherited, so a plain child of ablocked target computes
noneeven with the... *clause deleted, andnothing else on the page contests the specificity. With the fixture, collapsing
the repeated class or deleting the descendant clause each turns the tests red.
Three caveats worth knowing, since the bug has been latent since 2021:
canClickTarget: falseunder aclassPrefixand came torely on the target staying clickable will now find it blocked. The escape
hatch is
canClickTarget: true, or omitting the option.shepherd-target-click-disabledoutside a tour now getspointer-events: none. That class is Shepherd-internal and named forexactly this effect.
could. If
stepsContainerputs the popup inside the target, or an ancestorof the popup (
document.body, say) is listed inextraHighlights, thedescendant half of the rule kills the tour's own buttons. Those steps need
canClickTarget: true.One pre-existing leak is disclosed rather than silently carried. Calling
Step#show()directly — rather than throughTour#show, which hides thecurrent step first — re-resolves
extraHighlightsbefore tearing the stepdown, so elements that have since dropped out of the selector keep the classes
they were given. Under a prefix that leftover
shepherd-target-click-disabledused to be inert and is now permanent
pointer-events: none. The same callleaks
highlightClassand the prefixedshepherd-enabled/shepherd-targetidentically, so it is a
_showordering bug that predates this change andwants its own commit, not a widening of a CSS selector fix. The
attachTotarget itself is not affected:
_setupElementstears down whilestep.targetstill points at the old element, and only
setupTooltipreassigns it.landing/src/styles/shepherd.css is a hand-vendored copy of the minified
stylesheet that nothing in landing's build regenerates. Its selector is updated
here too, so shepherdjs.dev stops shipping a stylesheet that still has the bug.
Tours without a
classPrefixare entirely unaffected: the new selector matchesa strict superset of the old one at identical specificity.
Fixes #1298
Verification
types:checkclean.pointer-events: autoat 0-2-0 on the target, 0-1-0 on the child). Without that competition neither half of the rule is actually under test —pointer-eventsis inherited, so a plain child computesnoneeven with the... *clause deleted. With it, collapsing the repeated class or deleting the descendant clause each turns the tests red.Open since Jan 2021 and picked up by two contributors who both bounced off it. The sticking point is that a static stylesheet cannot know the runtime prefix, which is why this drops the prefixed classes from the selector rather than trying to prefix the CSS.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
canClickTargetis disabled, including child elements.classPrefixis configured.Documentation
classPrefix.canClickTarget: false.