-
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 all 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; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // jscpd:ignore-start | ||||||||||||||||||||||||
| const state = useStore<DBTabItemState>({ | ||||||||||||||||||||||||
| _selected: false, | ||||||||||||||||||||||||
|
|
@@ -49,17 +62,16 @@ export default function DBTabItem(props: DBTabItemProps) { | |||||||||||||||||||||||
| props.onChange(event); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // We have different ts types in different frameworks, so we need to use any here | ||||||||||||||||||||||||
milan-w marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| useTarget({ | ||||||||||||||||||||||||
| stencil: () => { | ||||||||||||||||||||||||
| const selected = (event.target as any)?.['checked']; | ||||||||||||||||||||||||
| state._selected = getBooleanAsString(selected); | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| default: () => { | ||||||||||||||||||||||||
| state._selected = (event.target as any)?.['checked']; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| if (_ref.checked && !state._selected) { | ||||||||||||||||||||||||
| useTarget({ | ||||||||||||||||||||||||
| stencil: () => { | ||||||||||||||||||||||||
| state._selected = getBooleanAsString(true); | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| default: () => { | ||||||||||||||||||||||||
| state._selected = true; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| }); | |
| }); | |
| } 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); | ||
| }); | ||
| }; | ||
|
|
||
|
|
||
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
setSelectedOnChangefunction comparesevent.target === _ref, but_refis a ref object, not the DOM element itself. To get the actual DOM element, you need to access_refdirectly (since in Mitosis, refs work differently per framework). The correct comparison should check if the event's target matches the actual input element.Additionally, this logic appears to deselect tabs when another is selected, but the condition
event.target === _refwill only be true for the newly selected tab, not the previously selected one. The function should check ifevent.target !== _refto properly deselect this tab when another tab is selected.