fix(list): focus pressed row on mousedown to keep clicks on target#4140
Conversation
With `delegatesFocus`, pressing a non-focusable part of a row (e.g. its text) delegated focus to the first focusable row and scrolled it into view. In a scrolled list this moved the pressed row out from under the pointer before mouseup, so the click landed on empty space and no item was selected. Focus the pressed row directly instead, without scrolling. Refs Lundalogik/crm-client#795 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
Changeslimel-list mousedown focus management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4140/ |
![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 `@src/components/list/list.e2e.tsx`:
- Around line 238-293: Add a new test case within the describe block for the
disabled row scenario to ensure the mousedown handler properly prevents focus on
disabled items. Create a test with items where at least one item has a disabled
property set to true, dispatch a mousedown event on the disabled item element,
then verify using an assertion that the shadowRoot.activeElement does not point
to that disabled item. This covers the explicit branch for disabled rows
mentioned in the handler implementation.
In `@src/components/list/list.tsx`:
- Around line 239-243: The focus gating check in the list component relies only
on the CSS class check
`itemElement.classList.contains('mdc-deprecated-list-item--disabled')` to
determine if an item is disabled, which is fragile since the aria-disabled
attribute is the source of truth for disabled state in list-item.tsx. Update the
disabled check to also verify the aria-disabled attribute on itemElement by
checking if the element does not have aria-disabled set to "true", either in
addition to or instead of the class-based check to ensure focus is properly
gated based on the accessibility attribute.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b1e3f53-01a9-4862-ae53-bfc1276cd26c
📒 Files selected for processing (2)
src/components/list/list.e2e.tsxsrc/components/list/list.tsx
| describe('is set as `radio`', () => { | ||
| it('emits change when a radio item is pressed', async () => { | ||
| const items = [{ text: 'item 1' }, { text: 'item 2' }]; | ||
| const { root, waitForChanges, spyOnEvent } = await render( | ||
| <limel-list type="radio" items={items}></limel-list> | ||
| ); | ||
| const changeSpy = spyOnEvent('change'); | ||
| await waitForChanges(); | ||
|
|
||
| const itemEls = | ||
| root.shadowRoot.querySelectorAll('limel-list-item'); | ||
| const target = itemEls[1] as HTMLElement; | ||
| target.dispatchEvent( | ||
| new MouseEvent('mousedown', { | ||
| bubbles: true, | ||
| composed: true, | ||
| cancelable: true, | ||
| }) | ||
| ); | ||
| target.click(); | ||
| await waitForChanges(); | ||
|
|
||
| expect(changeSpy).toHaveReceivedEventDetail({ | ||
| ...items[1], | ||
| selected: true, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('is set as `checkbox`', () => { | ||
| it('emits change when a checkbox item is pressed', async () => { | ||
| const items = [{ text: 'item 1' }, { text: 'item 2' }]; | ||
| const { root, waitForChanges, spyOnEvent } = await render( | ||
| <limel-list type="checkbox" items={items}></limel-list> | ||
| ); | ||
| const changeSpy = spyOnEvent('change'); | ||
| await waitForChanges(); | ||
|
|
||
| const itemEls = | ||
| root.shadowRoot.querySelectorAll('limel-list-item'); | ||
| const target = itemEls[1] as HTMLElement; | ||
| target.dispatchEvent( | ||
| new MouseEvent('mousedown', { | ||
| bubbles: true, | ||
| composed: true, | ||
| cancelable: true, | ||
| }) | ||
| ); | ||
| target.click(); | ||
| await waitForChanges(); | ||
|
|
||
| expect(changeSpy).toHaveReceivedEventDetail([ | ||
| { ...items[1], selected: true }, | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add an E2E assertion for disabled-row mousedown path.
The new handler has an explicit branch for disabled rows (prevent default, but no focus move), but current added tests only cover enabled rows. Add one test that dispatches mousedown on a disabled item and verifies shadowRoot.activeElement does not become that item.
🤖 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 `@src/components/list/list.e2e.tsx` around lines 238 - 293, Add a new test case
within the describe block for the disabled row scenario to ensure the mousedown
handler properly prevents focus on disabled items. Create a test with items
where at least one item has a disabled property set to true, dispatch a
mousedown event on the disabled item element, then verify using an assertion
that the shadowRoot.activeElement does not point to that disabled item. This
covers the explicit branch for disabled rows mentioned in the handler
implementation.
| if ( | ||
| !itemElement.classList.contains( | ||
| 'mdc-deprecated-list-item--disabled' | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
Use ARIA-disabled check for focus gating instead of CSS class-only check.
At Line 240, the non-disabled check relies on mdc-deprecated-list-item--disabled. In src/components/list-item/list-item.tsx (Lines 130-190), disabled state is explicitly exposed via aria-disabled, so this gate is safer if based on that attribute (or both class + attribute) to avoid focusing disabled rows when class wiring changes.
Suggested patch
- if (
- !itemElement.classList.contains(
- 'mdc-deprecated-list-item--disabled'
- )
- ) {
+ const isDisabled =
+ itemElement.getAttribute('aria-disabled') === 'true' ||
+ itemElement.classList.contains(
+ 'mdc-deprecated-list-item--disabled'
+ );
+ if (!isDisabled) {
itemElement.focus({ preventScroll: true });
}📝 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.
| if ( | |
| !itemElement.classList.contains( | |
| 'mdc-deprecated-list-item--disabled' | |
| ) | |
| ) { | |
| const isDisabled = | |
| itemElement.getAttribute('aria-disabled') === 'true' || | |
| itemElement.classList.contains( | |
| 'mdc-deprecated-list-item--disabled' | |
| ); | |
| if (!isDisabled) { | |
| itemElement.focus({ preventScroll: true }); | |
| } |
🤖 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 `@src/components/list/list.tsx` around lines 239 - 243, The focus gating check
in the list component relies only on the CSS class check
`itemElement.classList.contains('mdc-deprecated-list-item--disabled')` to
determine if an item is disabled, which is fragile since the aria-disabled
attribute is the source of truth for disabled state in list-item.tsx. Update the
disabled check to also verify the aria-disabled attribute on itemElement by
checking if the element does not have aria-disabled set to "true", either in
addition to or instead of the class-based check to ensure focus is properly
gated based on the accessibility attribute.
devbymadde
left a comment
devbymadde
left a comment
There was a problem hiding this comment.
Great work, tested it locally and it works well! Code looks good, let's ship it :D
|
🎉 This PR is included in version 39.34.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
fix https://github.com/Lundalogik/crm-client/issues/795
Summary
limel-listwas lost:delegatesFocusredirected focus to the first (off-screen) row and scrolled it into view, moving the pressed row out from under the pointer so mouseup landed on empty<ul>space and MDC computed index-1. This is the cause of the object explorer widget "jumps to top instead of opening the clicked object" bug.mousedown, suppress the default focus delegation and focus the pressed row with{ preventScroll: true }. Mouse-only (notpointerdown) so touch-drag scrolling isn't blocked;preventDefaultfor any row (so disabled rows don't scroll-jump either), but focus only moves to non-disabled rows.radio/checkboxtoggling and pressed-row focus.Test plan
npx stencil-test --project e2e list/list.e2epasses (15/15)radioandcheckboxlist examples still toggle on clicklimel-menu-liststill activates items (shares the pattern; not changed here)Refs Lundalogik/crm-client#795
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes