fix(core): make present-mode click-to-navigate pass through to slide content#179
Conversation
…content Replace the two invisible w-[30%] nav button overlays with a position-based click handler on the player root. The overlays sat on top of the slide and blocked clicks to any interactive content (embedded videos, links, buttons) within ~30% of either edge. The handler reads the click X position (outer 30% prev/next, inert center) and bails when the target is interactive or part of the present chrome, so content stays clickable everywhere. This also removes the stray focus outline that flashed on the buttons. Navigation keeps full keyboard parity via the global keydown handler and the control bar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughHook-driven edge click navigation added and wired into Player and SlideViewportNavigation; mobile gating and chrome marking prevent accidental advances. Locale keys and Locale type for ChangesClick-to-Navigate Edge Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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📝 Generate docstrings
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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/app/components/click-nav-zones.tsx`:
- Line 25: Replace the Tailwind class that removes outlines with the
accessibility-friendly variant: in the click-nav button components (look for the
className string containing "absolute inset-y-0 left-0 z-20 w-[18%] min-w-12
outline-none md:hidden" and the corresponding right-side button) change
"outline-none" to "outline-hidden" so forced-colors/high-contrast modes still
show outlines while keeping the visual hide in normal themes.
In `@packages/core/src/app/components/player.tsx`:
- Around line 34-35: The NAV_PASSTHROUGH selector currently only matches
[contenteditable="true"] and thus misses other editable states like
contenteditable="plaintext-only"; update NAV_PASSTHROUGH to match any editable
element except an explicit false by replacing [contenteditable="true"] with a
broader selector such as [contenteditable]:not([contenteditable="false"]) in the
NAV_PASSTHROUGH constant so editable regions in plaintext-only or other editable
modes are included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4084a8cc-b722-4e76-959e-c70ec66eabfd
📒 Files selected for processing (3)
.changeset/fix-present-edge-click-nav.mdpackages/core/src/app/components/click-nav-zones.tsxpackages/core/src/app/components/player.tsx
| const NAV_PASSTHROUGH = | ||
| 'a, button, input, textarea, select, label, summary, iframe, video, audio, embed, object, [role="button"], [role="link"], [contenteditable="true"], [data-osd-interactive], [data-osd-chrome]'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the valid HTML values for the contenteditable attribute, and does [contenteditable="true"] match elements declared as <div contenteditable> or contenteditable="plaintext-only"?
💡 Result:
The contenteditable attribute is an enumerated attribute, meaning it must take one of a specific set of keyword values [1][2][3]. Valid values for the contenteditable attribute include: - true: Indicates that the element is editable [2][4]. - false: Indicates that the element is not editable [2][4]. - plaintext-only: Indicates that the element's raw text is editable, but rich text formatting is disabled [2][3][5]. - "" (empty string): Treated as equivalent to true [2][3]. Note that although "true" and "false" are allowed values, contenteditable is an enumerated attribute, not a boolean attribute [2][3]. If the attribute is missing, or if an invalid value is provided, the element inherits its editability state from its parent [2][4][6]. Regarding your second question: Yes, [contenteditable="true"] will match elements declared as
Citations:
- 1: https://www.w3.org/TR/content-editable/
- 2: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/contenteditable
- 3: https://mdn.org.cn/docs/Web/HTML/Global_attributes/contenteditable
- 4: https://www.w3schools.com/tags/att_global_contenteditable.asp
- 5: https://web-platform-dx.github.io/web-features-explorer/features/contenteditable-plaintextonly/
- 6: https://www.tiny.cloud/blog/using-html-contenteditable/
- 7: https://web.dev/blog/contenteditable-plaintext-only-baseline
Broaden NAV_PASSTHROUGH’s contenteditable selector to include plaintext-only
[contenteditable="true"] doesn’t match the contenteditable="plaintext-only" state, so editable regions in that mode can still trigger navigation. Update the selector to include all editability states except an explicit false:
Suggested fix
const NAV_PASSTHROUGH =
- 'a, button, input, textarea, select, label, summary, iframe, video, audio, embed, object, [role="button"], [role="link"], [contenteditable="true"], [data-osd-interactive], [data-osd-chrome]';
+ 'a, button, input, textarea, select, label, summary, iframe, video, audio, embed, object, [role="button"], [role="link"], [contenteditable]:not([contenteditable="false"]), [data-osd-interactive], [data-osd-chrome]';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const NAV_PASSTHROUGH = | |
| 'a, button, input, textarea, select, label, summary, iframe, video, audio, embed, object, [role="button"], [role="link"], [contenteditable="true"], [data-osd-interactive], [data-osd-chrome]'; | |
| const NAV_PASSTHROUGH = | |
| 'a, button, input, textarea, select, label, summary, iframe, video, audio, embed, object, [role="button"], [role="link"], [contenteditable]:not([contenteditable="false"]), [data-osd-interactive], [data-osd-chrome]'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/app/components/player.tsx` around lines 34 - 35, The
NAV_PASSTHROUGH selector currently only matches [contenteditable="true"] and
thus misses other editable states like contenteditable="plaintext-only"; update
NAV_PASSTHROUGH to match any editable element except an explicit false by
replacing [contenteditable="true"] with a broader selector such as
[contenteditable]:not([contenteditable="false"]) in the NAV_PASSTHROUGH constant
so editable regions in plaintext-only or other editable modes are included.
… viewer Extract the position-based click navigation into a single useClickPageNavigation hook (interactive-content passthrough + inert center) and use it in both the Player and the slide viewer. The viewer's mobile tap zones replace the old ClickNavZones button overlays, so embedded videos/links near the edges stay tappable there too. Removes the now-dead component and its aria-label locale keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/app/lib/use-click-page-navigation.ts`:
- Around line 33-37: The NAV_PASSTHROUGH guard in onClick incorrectly narrows
event.target to HTMLElement so inline SVG clicks (SVGElement) bypass the closest
check; change the type check from "target instanceof HTMLElement" to "target
instanceof Element" (or cast event.target to Element) so
target.closest(NAV_PASSTHROUGH) runs for SVG elements as well; update
use-click-page-navigation's onClick to use Element for the guard and keep the
same NAV_PASSTHROUGH constant and logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edfffa05-ffd0-4262-b7a1-b5f878c62cc7
📒 Files selected for processing (11)
.changeset/fix-present-edge-click-nav.mdpackages/core/src/app/components/click-nav-zones.tsxpackages/core/src/app/components/player.tsxpackages/core/src/app/lib/use-click-page-navigation.tspackages/core/src/app/lib/use-is-mobile.tspackages/core/src/app/routes/slide.tsxpackages/core/src/locale/en.tspackages/core/src/locale/ja.tspackages/core/src/locale/types.tspackages/core/src/locale/zh-cn.tspackages/core/src/locale/zh-tw.ts
💤 Files with no reviewable changes (6)
- packages/core/src/locale/zh-cn.ts
- packages/core/src/locale/en.ts
- packages/core/src/app/components/click-nav-zones.tsx
- packages/core/src/locale/ja.ts
- packages/core/src/locale/types.ts
- packages/core/src/locale/zh-tw.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-present-edge-click-nav.md
| const onClick = (event: MouseEvent) => { | ||
| if (event.button !== 0 || event.defaultPrevented) return; | ||
| const target = event.target; | ||
| if (target instanceof HTMLElement && target.closest(NAV_PASSTHROUGH)) return; | ||
| if (window.getSelection()?.toString()) return; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Element.closest() available on SVGElement, and is event.target an SVGElement when clicking an inline SVG inside a button?
💡 Result:
Yes, Element.closest is available on SVGElement [1]. Because SVGElement inherits from the Element interface [2], it possesses all standard Element methods, including closest [3]. When you click an inline SVG nested inside a button, event.target is indeed the specific element you clicked—which will frequently be the SVGElement itself (or one of its children like a path or rect element) rather than the parent button element [4][5][6]. To reliably reference the button, you should use event.currentTarget instead of event.target [4][7]. The currentTarget property always refers to the element to which the event handler is attached, regardless of which descendant was actually clicked [4]. Alternatively, you can apply pointer-events: none; in CSS to the SVG or its children, which causes the browser to ignore the SVG for pointer events, allowing the click to pass through to the button [6][8].
Citations:
- 1: https://docs.mobilizing-js.net/interfaces/_internal_.SVGElement.html
- 2: https://developer.mozilla.org/en-US/docs/Web/API/SVGElement
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
- 4: https://stackoverflow.com/questions/63577645/js-click-event-capturing-down-to-svg-instead-of-button
- 5: https://css-tricks.com/links-inline-svg-staying-target-events/
- 6: https://schalkneethling.medium.com/preventing-svg-from-becoming-the-event-target-3e9c039dfdd0
- 7: https://stackoverflow.com/questions/79482870/vue-js-unable-to-get-values-from-button-if-svg-is-present
- 8: https://stackoverflow.com/questions/2296097/making-an-svg-image-object-clickable-with-onclick-avoiding-absolute-positioning
Fix NAV_PASSTHROUGH guard to handle inline SVG clicks
When clicking an inline <svg> (or its child like <path>), event.target is typically an SVGElement, not an HTMLElement, so the current instanceof HTMLElement check skips NAV_PASSTHROUGH and navigation can fire on interactive icon content. SVGElement inherits from Element, so closest works with a widened Element check.
🐛 Proposed fix
const target = event.target;
- if (target instanceof HTMLElement && target.closest(NAV_PASSTHROUGH)) return;
+ if (target instanceof Element && target.closest(NAV_PASSTHROUGH)) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onClick = (event: MouseEvent) => { | |
| if (event.button !== 0 || event.defaultPrevented) return; | |
| const target = event.target; | |
| if (target instanceof HTMLElement && target.closest(NAV_PASSTHROUGH)) return; | |
| if (window.getSelection()?.toString()) return; | |
| const onClick = (event: MouseEvent) => { | |
| if (event.button !== 0 || event.defaultPrevented) return; | |
| const target = event.target; | |
| if (target instanceof Element && target.closest(NAV_PASSTHROUGH)) return; | |
| if (window.getSelection()?.toString()) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/app/lib/use-click-page-navigation.ts` around lines 33 - 37,
The NAV_PASSTHROUGH guard in onClick incorrectly narrows event.target to
HTMLElement so inline SVG clicks (SVGElement) bypass the closest check; change
the type check from "target instanceof HTMLElement" to "target instanceof
Element" (or cast event.target to Element) so target.closest(NAV_PASSTHROUGH)
runs for SVG elements as well; update use-click-page-navigation's onClick to use
Element for the guard and keep the same NAV_PASSTHROUGH constant and logic.
1 participant
Problem
In present mode the left/right click-to-navigate zones were two invisible
w-[30%]<button>overlays sitting on top of the slide (z-10). Any interactive slide content — embedded videos, links, buttons — within ~30% of either edge had its clicks swallowed by the overlay and was effectively unclickable. The overlays also flashed a stray focus outline (the globaloutline-ring/50rule) on click.Fix
Remove both overlay buttons and handle click-to-navigate via a position-based handler on the player root:
a, button, input, iframe, video, audio, embed, object, [role=button],[contenteditable], …) or part of the present chrome (marked withdata-osd-chrome), so content stays clickable everywhere.data-osd-interactive.Because nothing covers the slide anymore, the embedded-video case works (cross-origin iframe clicks never reach the parent), and the outline-flash bug disappears with the buttons.
Navigation keeps full keyboard parity (global keydown: arrows / space / PageUp-Down) and the control bar's prev/next buttons — hence the two suppressed a11y lints on the root div.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Chores