Standardise ledger scrolling when using keyboard shortcuts#7283
Standardise ledger scrolling when using keyboard shortcuts#7283JSkinnerUK wants to merge 4 commits intoactualbudget:masterfrom
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
📝 WalkthroughWalkthroughUpdated the table navigator's keydown handler to call e.preventDefault() for ArrowUp/ArrowDown and Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/desktop-client/src/components/table.tsx (1)
1366-1398:⚠️ Potential issue | 🟠 MajorScope
preventDefault()to handled navigation keys only.At Line 1366,
preventDefault()now runs for every key that reaches the table handler, which can suppress unrelated default keyboard behavior while focus is inside the table. This broadens behavior beyond the navigation fix.Suggested patch
onKeyDown: e => { userProps?.onKeyDown?.(e); if (e.isPropagationStopped()) { return; } - e.preventDefault(); + const shouldPreventDefault = + e.key === 'ArrowUp' || + e.key === 'ArrowDown' || + e.key === 'k' || + e.key === 'j' || + e.key === 'Enter' || + e.key === 'Tab'; + if (shouldPreventDefault) { + e.preventDefault(); + } switch (e.key) { case 'ArrowUp': case 'k': if (e.target.tagName !== 'INPUT') { onMove('up'); } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/table.tsx` around lines 1366 - 1398, The keydown handler currently calls e.preventDefault() unconditionally at the top, which blocks all default behaviors; restrict preventDefault() to only the navigation keys handled in the switch (the 'ArrowUp'/'k', 'ArrowDown'/'j', 'Enter', and 'Tab' cases). Move or add e.preventDefault() into each handled branch (or just before calling onMove) so that only when you invoke onMove('up'|'down'|'left'|'right') the default is prevented; leave other keys untouched. Use the existing symbols (the keydown handler that references e.key and onMove) to locate where to limit preventDefault().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/desktop-client/src/components/table.tsx`:
- Around line 1366-1398: The keydown handler currently calls e.preventDefault()
unconditionally at the top, which blocks all default behaviors; restrict
preventDefault() to only the navigation keys handled in the switch (the
'ArrowUp'/'k', 'ArrowDown'/'j', 'Enter', and 'Tab' cases). Move or add
e.preventDefault() into each handled branch (or just before calling onMove) so
that only when you invoke onMove('up'|'down'|'left'|'right') the default is
prevented; leave other keys untouched. Use the existing symbols (the keydown
handler that references e.key and onMove) to locate where to limit
preventDefault().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49580719-f778-4a20-8e4b-e333ad2a91ca
📒 Files selected for processing (2)
packages/desktop-client/src/components/table.tsxupcoming-release-notes/7283.md
|
VRT tests ❌ failed. View the test report. To update the VRT screenshots, comment |
JSkinnerUK
force-pushed
the
ledger-scrolling
branch
from
b23bd2b to
af1f216
Compare
JSkinnerUK
force-pushed
the
ledger-scrolling
branch
2 times, most recently
from
347e0a9 to
8a8db1a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/desktop-client/src/components/table.tsx (1)
1366-1400:⚠️ Potential issue | 🟠 MajorUnconditional
preventDefaultis over-broad and can suppress unrelated keyboard behaviorLine 1366 cancels default behavior for every key event that reaches this handler, including keys not handled in the switch (
defaultcase). ScopepreventDefault()to only the navigation keys you actually process.🔧 Suggested fix
onKeyDown: e => { userProps?.onKeyDown?.(e); if (e.isPropagationStopped()) { return; } - - e.preventDefault(); switch (e.key) { case 'ArrowUp': case 'k': if (e.target.tagName !== 'INPUT') { e.preventDefault(); onMove('up'); } break; case 'ArrowDown': case 'j': if (e.target.tagName !== 'INPUT') { e.preventDefault(); onMove('down'); } break; case 'Enter': case 'Tab': + e.preventDefault(); e.stopPropagation(); onMove( e.key === 'Enter' ? e.shiftKey ? 'up' : 'down' : e.shiftKey ? 'left' : 'right', ); break; default: + return; } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/table.tsx` around lines 1366 - 1400, The current handler calls e.preventDefault() unconditionally which suppresses unrelated key behavior; remove the top-level e.preventDefault() and instead invoke e.preventDefault() only inside the handled branches (the ArrowUp/'k' and ArrowDown/'j' branches when e.target.tagName !== 'INPUT', and for Enter/Tab before calling onMove), preserving the existing e.stopPropagation() on Enter/Tab and the existing direction logic that uses onMove; update the switch in the table key handler so the default case does nothing and only navigation keys prevent default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/desktop-client/src/components/table.tsx`:
- Around line 1366-1400: The current handler calls e.preventDefault()
unconditionally which suppresses unrelated key behavior; remove the top-level
e.preventDefault() and instead invoke e.preventDefault() only inside the handled
branches (the ArrowUp/'k' and ArrowDown/'j' branches when e.target.tagName !==
'INPUT', and for Enter/Tab before calling onMove), preserving the existing
e.stopPropagation() on Enter/Tab and the existing direction logic that uses
onMove; update the switch in the table key handler so the default case does
nothing and only navigation keys prevent default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 481f6dac-f151-4589-899e-12e3765b90ad
📒 Files selected for processing (1)
packages/desktop-client/src/components/table.tsx
JSkinnerUK
force-pushed
the
ledger-scrolling
branch
2 times, most recently
from
8a8db1a to
733c6d9
Compare
JSkinnerUK
changed the title
2 participants
Description
Depending on whether navigating with the arrow keys, or k/j, the scrolling would be inconsistent, due to the arrow keys interacting with the browsers scroll.
e.preventDefault()was already used in the other navigation cases, and applying to all cases will give consistent behaviour.Related issue(s)
Fixes #940
Testing
Recreating steps are in #940. I tested navigating with up and down arrows, k/j, enter and shift enter, and tab and shift tab.
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/components/table.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
api
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged