Skip to content
6 changes: 6 additions & 0 deletions .changeset/sharp-pumas-obey.md
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
63 changes: 49 additions & 14 deletions packages/components/src/components/tab-item/tab-item.lite.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
onMount,
onUnMount,
onUpdate,
Show,
useDefaultProps,
Expand Down Expand Up @@ -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;
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setSelectedOnChange function compares event.target === _ref, but _ref is a ref object, not the DOM element itself. To get the actual DOM element, you need to access _ref directly (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 === _ref will only be true for the newly selected tab, not the previously selected one. The function should check if event.target !== _ref to properly deselect this tab when another tab is selected.

Suggested change
state._selected = event.target === _ref;
state._selected = event.target === _ref.current;

Copilot uses AI. Check for mistakes.
}
});
}

// jscpd:ignore-start
const state = useStore<DBTabItemState>({
_selected: false,
Expand All @@ -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

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;
}
});
Copy link

Copilot AI Nov 10, 2025

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.

Suggested change
});
});
} else if (!_ref.checked && state._selected) {
useTarget({
stencil: () => {
state._selected = getBooleanAsString(false);
},
default: () => {
state._selected = false;
}
});

Copilot uses AI. Check for mistakes.
}

useTarget({
angular: () =>
Expand All @@ -76,12 +88,26 @@ export default function DBTabItem(props: DBTabItemProps) {

onUpdate(() => {
if (state.initialized && _ref) {
useTarget({ react: () => state.handleNameAttribute() });
state.initialized = false;

// deselect this tab when another tab in tablist is selected
_ref.closest('[role=tablist]')?.addEventListener(
'change',
setSelectedOnChange
);
Comment on lines +94 to +98
Copy link

Copilot AI Nov 10, 2025

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

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

Copy link
Collaborator

@mfranzke mfranzke Nov 13, 2025

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.


if (props.active) {
useTarget({
stencil: () => {
state._selected = getBooleanAsString(true);
},
default: () => {
state._selected = true;
}
});
_ref.click();
}

useTarget({ react: () => state.handleNameAttribute() });
state.initialized = false;
}
}, [_ref, state.initialized]);

Expand All @@ -91,6 +117,15 @@ export default function DBTabItem(props: DBTabItemProps) {
}
}, [props.name]);

onUnMount(() => {
if (state.initialized && _ref) {
Copy link

Copilot AI Nov 10, 2025

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.

Suggested change
if (state.initialized && _ref) {
if (_ref) {

Copilot uses AI. Check for mistakes.
_ref.closest('[role=tablist]')?.removeEventListener(
'change',
setSelectedOnChange
);
}
});

return (
<li class={cls('db-tab-item', props.className)} role="none">
<label
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/components/tabs/tabs.lite.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ export default function DBTabs(props: DBTabsProps) {
if (tabItem) {
const list = tabItem.parentElement;
if (list) {
const indices = Array.from(list.childNodes).indexOf(
const tabIndex = Array.from(list.children).indexOf(
tabItem
);
if (props.onIndexChange) {
props.onIndexChange(indices);
props.onIndexChange(tabIndex);
}

if (props.onTabSelect) {
Expand Down
17 changes: 7 additions & 10 deletions packages/components/src/components/tabs/tabs.spec.tsx
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;
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tabIndex variable is declared at module scope, which means its value persists across test runs. This can cause test pollution where one test's execution affects another test's state.

Before line 43 checks expect(tabIndex).toBe(undefined), if another test has already run and set tabIndex, this assertion will fail. Additionally, the variable is never reset between tests.

Consider either:

  1. Declaring tabIndex inside the test function and passing it via closure
  2. Using test.beforeEach() to reset the variable
  3. Making the variable test-specific using a different approach (e.g., a Map keyed by test context)

Copilot uses AI. Check for mistakes.

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>
Expand Down Expand Up @@ -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')
Expand All @@ -59,7 +56,7 @@ const testActions = () => {
.isChecked();
expect(!tabChecked).toBeTruthy();

expect(existsSync(filePath)).toBeTruthy();
expect(tabIndex).toBe(1);
});
};

Expand Down
Loading