-
Notifications
You must be signed in to change notification settings - Fork 13
fix: set DBTabItem selected state correctly #5399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
73851f7
ee4f087
fb22490
83dda9d
91f9c90
01772af
d1535e1
d3c5b8d
acf8201
c05f2a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@db-ux/core-components": patch | ||
| --- | ||
|
|
||
| fix: set DBTabItem internal state `_selected` correctly | ||
| - now also sets aria-selected=true|false correctly which improves screen reader behaviour | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| onMount, | ||||||||||||||||||||||||
| onUnMount, | ||||||||||||||||||||||||
| onUpdate, | ||||||||||||||||||||||||
| Show, | ||||||||||||||||||||||||
| useDefaultProps, | ||||||||||||||||||||||||
|
|
@@ -27,6 +28,18 @@ useDefaultProps<DBTabItemProps>({}); | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export default function DBTabItem(props: DBTabItemProps) { | ||||||||||||||||||||||||
| const _ref = useRef<HTMLInputElement | any>(null); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function setSelectedOnChange(event: any) { | ||||||||||||||||||||||||
| useTarget({ | ||||||||||||||||||||||||
| stencil: () => { | ||||||||||||||||||||||||
| state._selected = getBooleanAsString(event.target === _ref); | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| default: () => { | ||||||||||||||||||||||||
| state._selected = event.target === _ref; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| state._selected = event.target === _ref; | |
| state._selected = event.target === _ref.current; |
milan-w marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition if (_ref.checked && !state._selected) only updates state._selected to true when the tab becomes checked. However, there's no corresponding logic to set state._selected to false when the tab becomes unchecked.
The new setSelectedOnChange function (lines 32-41) is supposed to handle deselection, but as noted in another comment, it has a logic issue. Consider adding an else clause here to explicitly set state._selected to false when _ref.checked is false, ensuring the state is always synchronized with the checked status.
| }); | |
| }); | |
| } else if (!_ref.checked && state._selected) { | |
| useTarget({ | |
| stencil: () => { | |
| state._selected = getBooleanAsString(false); | |
| }, | |
| default: () => { | |
| state._selected = false; | |
| } | |
| }); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event listener is added in onUpdate every time state.initialized becomes true and _ref exists. However, there's no removal of the previous listener before adding a new one. Since state.initialized is set to true in onMount and then set to false after the update, subsequent updates could potentially add multiple listeners to the same element, causing the handler to fire multiple times.
Consider tracking whether the listener has been added (e.g., with a flag) or ensuring the listener is only added once, or removing it before re-adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this is an excellent point and I'm angry I didn't catch it myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI is here to hurt your feelings.
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onUnMount hook checks state.initialized, but this is incorrect. At the time of unmounting, state.initialized is likely false because it's set to false in the onUpdate hook after initialization completes (line 92). This means the event listener cleanup in onUnMount will likely never execute, causing a memory leak.
The condition should either check if _ref exists, or remove the state.initialized check entirely since _ref should be sufficient to determine if the event listener was attached.
| if (state.initialized && _ref) { | |
| if (_ref) { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,17 @@ | ||
| import AxeBuilder from '@axe-core/playwright'; | ||
| import { expect, test } from '@playwright/experimental-ct-react'; | ||
|
|
||
| import { existsSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { DBTabs } from './index'; | ||
| // @ts-ignore - vue can only find it with .ts as file ending | ||
| import { DEFAULT_VIEWPORT } from '../../shared/constants.ts'; | ||
| import { DBTabItem } from '../tab-item'; | ||
| import { DBTabList } from '../tab-list'; | ||
| import { DBTabPanel } from '../tab-panel'; | ||
|
|
||
| const filePath = './test-results/onIndexChange.txt'; | ||
| let tabIndex: number; | ||
|
||
|
|
||
| const comp: any = ( | ||
| <DBTabs | ||
| onIndexChange={(index: number) => | ||
| writeFileSync(filePath, index.toString()) | ||
| }> | ||
| <DBTabs onIndexChange={(index: number) => (tabIndex = index)}> | ||
| <DBTabList> | ||
| <DBTabItem data-testid="test">Test 1</DBTabItem> | ||
| <DBTabItem data-testid="test2">Test 2</DBTabItem> | ||
|
|
@@ -44,10 +40,11 @@ const testComponent = () => { | |
|
|
||
| const testActions = () => { | ||
| test('should be clickable', async ({ mount }) => { | ||
| if (existsSync(filePath)) { | ||
| rmSync(filePath); | ||
| } | ||
| expect(tabIndex).toBe(undefined); | ||
|
|
||
| // Beware: the comments below actually change the selector for vue | ||
| // this is necessary because vue will not trigger a check on an list element but requires the actual | ||
| // radio button element, which has the role=tab | ||
| const component = await mount(comp); | ||
| await component | ||
| .getByTestId('test2') | ||
|
|
@@ -59,7 +56,7 @@ const testActions = () => { | |
| .isChecked(); | ||
| expect(!tabChecked).toBeTruthy(); | ||
|
|
||
| expect(existsSync(filePath)).toBeTruthy(); | ||
| expect(tabIndex).toBe(1); | ||
| }); | ||
| }; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.