Show modal if there is no room for shortcuts - #732
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds shared docked-versus-modal behavior for shortcuts and player info panels, based on narrow layout and available docked height. It updates modal accessibility handling, focus behavior, and resize observation, and positions landscape tabs alongside the canvas using a CSS width variable. ChangesResponsive info panels
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant InfoPanelTabs
participant ShortcutsPanel
participant ModalPanelBehaviour
participant PanelDOM
User->>InfoPanelTabs: activate panel tab
InfoPanelTabs->>ShortcutsPanel: owner.toggle()
ShortcutsPanel->>ModalPanelBehaviour: shouldBeModal()
ModalPanelBehaviour-->>ShortcutsPanel: modal or docked state
ShortcutsPanel->>PanelDOM: show panel and update accessibility attributes
User->>PanelDOM: resize window or docked content
PanelDOM->>ModalPanelBehaviour: resize notification
ModalPanelBehaviour->>ShortcutsPanel: enterModal() or exitModal()
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
accessibility/keyboardui.js (1)
1277-1283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape handler and
hide()fight over focus.
hide()already restorespreviousFocus; this then re-focuses the tab button, so the restore is wasted work and the final target differs depending on layout. Consider lettinghide()own focus restoration and dropping the extra focus call (or vice versa) so there's a single rule.🤖 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 `@accessibility/keyboardui.js` around lines 1277 - 1283, Remove the tab-button lookup and focus call from the Escape-key handler, allowing hide() to exclusively restore previousFocus. Keep the existing preventDefault, stopPropagation, and hide() behavior unchanged.style.css (1)
1140-1150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic
52pxbottom-bar height.
bottom: calc(52px + …)silently depends on the bottom bar's height. A shared custom property (or--bottombar-height) would keep the tabs aligned if that bar changes.🤖 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 `@style.css` around lines 1140 - 1150, Replace the hardcoded 52px offset in aside#info-panel `#info-panel-tabs` with the shared bottom-bar height custom property, preserving the safe-area inset calculation and existing positioning behavior. Ensure the property is defined and reused from the bottom-bar styling rather than introducing a duplicate height value.
🤖 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 `@accessibility/keyboardui.js`:
- Around line 774-785: Update the focus restoration in watchDockedSpace so
previousFocus is focused only when the current active element is actually inside
the modal or is the removed close-button control; otherwise preserve focus on
surviving panel controls. Clear or consume previousFocus after restoring focus
so a later hide() cannot refocus stale state.
- Around line 1268-1276: Update the ArrowUp/ArrowDown handling around the
scroller lookup so it targets the container that is actually scrolling: use the
modal panel’s active scroll container when the panel is reparented to
document.body, and retain `#info-panel-body` for docked mode. Preserve
preventDefault and the existing ±100 instant scroll behavior.
In `@main/view.js`:
- Around line 211-214: Update resizeCanvas() so the --canvas-width CSS variable
is set from canvas.getBoundingClientRect().right rather than newWidth, ensuring
fixed info-panel tabs anchor to the canvas’s actual right edge. Preserve the
existing rounding and CSS property publication.
---
Nitpick comments:
In `@accessibility/keyboardui.js`:
- Around line 1277-1283: Remove the tab-button lookup and focus call from the
Escape-key handler, allowing hide() to exclusively restore previousFocus. Keep
the existing preventDefault, stopPropagation, and hide() behavior unchanged.
In `@style.css`:
- Around line 1140-1150: Replace the hardcoded 52px offset in aside#info-panel
`#info-panel-tabs` with the shared bottom-bar height custom property, preserving
the safe-area inset calculation and existing positioning behavior. Ensure the
property is defined and reused from the bottom-bar styling rather than
introducing a duplicate height value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5623ac2d-60fa-4ea1-bfb2-a380627e7f70
📒 Files selected for processing (3)
accessibility/keyboardui.jsmain/view.jsstyle.css
1 participant
Summary
If there is no room below the canvas to show the contents of the keyboard or on-screen controls documentation tabs, show them in a modal instead. This previously happened in certain cases but didn't properly measure the height available. It should now go modal in all cases where it makes sense.
Also moved the tabs for these documentation panels under the gizmos on mobile landscape otherwise you couldn't even use them.
AI usage
Claude Haiku, Sonnet and Opus used for varying bits. Plans made and tested by me.
Summary by CodeRabbit
New Features
Accessibility
Bug Fixes