Skip to content

Commit f7625cb

Browse files
milan-wgithub-actions[bot]
milan-w
and
github-actions[bot]
authored
fix: set DBTabItem selected state correctly (#5399)
* fix: set DBTabItem selected state correctly Set (aria-)selected state via listener on parent tablist to also react on de-selection of item. * chore: update DBTab test use var instead of file to hold click event results for test duration * Fix DBTabItem selected state and aria-selected attribute Now also sets aria-selected=true|false correctly which improves screen reader behaviour. * auto update snapshots (#5400) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix: pass correct tab item index from DBTab parent to child set child active after event registered, remove event listerner on unmount * auto update snapshots (#5409) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * refactor: ensure active/selected state and aria-selected are in snyc add more checks and guardrails to TabItem on change events * fix: re-add stencil special boolean treatment in DBTabItem --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent 0cd6c2a commit f7625cb

File tree

5 files changed

+99
-42
lines changed

5 files changed

+99
-42
lines changed

‎.changeset/sharp-pumas-obey.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@db-ux/core-components": patch
3+
---
4+
5+
fix: set DBTabItem internal state `_selected` correctly
6+
- Now also sets aria-selected=true|false correctly which improves screen reader behaviour

‎packages/components/src/components/tab-item/model.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ export type DBTabItemProps = GlobalProps &
4949

5050
export type DBTabItemDefaultState = {
5151
_selected: boolean;
52+
_listenerAdded: boolean;
53+
boundSetSelectedOnChange?: (event: any) => void;
54+
setSelectedOnChange: (event: any) => void;
5255
};
5356

5457
export type DBTabItemState = DBTabItemDefaultState &

‎packages/components/src/components/tab-item/tab-item.lite.tsx‎

Lines changed: 66 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
onMount,
3+
onUnMount,
34
onUpdate,
45
Show,
56
useDefaultProps,
@@ -27,11 +28,25 @@ useDefaultProps<DBTabItemProps>({});
2728

2829
export default function DBTabItem(props: DBTabItemProps) {
2930
const _ref = useRef<HTMLInputElement | any>(null);
31+
3032
// jscpd:ignore-start
3133
const state = useStore<DBTabItemState>({
3234
_selected: false,
3335
_name: undefined,
3436
initialized: false,
37+
_listenerAdded: false,
38+
boundSetSelectedOnChange: undefined,
39+
setSelectedOnChange: (event: any) => {
40+
event.stopPropagation();
41+
useTarget({
42+
stencil: () => {
43+
state._selected = getBooleanAsString(event.target === _ref);
44+
},
45+
default: () => {
46+
state._selected = event.target === _ref;
47+
}
48+
});
49+
},
3550
handleNameAttribute: () => {
3651
if (_ref) {
3752
const setAttribute = _ref.setAttribute;
@@ -44,23 +59,10 @@ export default function DBTabItem(props: DBTabItemProps) {
4459
}
4560
},
4661
handleChange: (event: any) => {
47-
event.stopPropagation();
4862
if (props.onChange) {
4963
props.onChange(event);
5064
}
5165

52-
// We have different ts types in different frameworks, so we need to use any here
53-
54-
useTarget({
55-
stencil: () => {
56-
const selected = (event.target as any)?.['checked'];
57-
state._selected = getBooleanAsString(selected);
58-
},
59-
default: () => {
60-
state._selected = (event.target as any)?.['checked'];
61-
}
62-
});
63-
6466
useTarget({
6567
angular: () =>
6668
handleFrameworkEventAngular(state, event, 'checked'),
@@ -69,28 +71,73 @@ export default function DBTabItem(props: DBTabItemProps) {
6971
}
7072
});
7173

74+
// Set up event listener to react on any change (select & deselect) in tab list
75+
// Default: Most framework can just pass the state function to the parents event listener.
76+
// Stencil: Bind the function to maintain correct 'this' context in class components
77+
// React: Wrap in arrow function so setState doesn't treat it as a state updater
7278
onMount(() => {
79+
useTarget({
80+
stencil: () => {
81+
state.boundSetSelectedOnChange =
82+
state.setSelectedOnChange.bind(state);
83+
},
84+
react: () => {
85+
state.boundSetSelectedOnChange = () =>
86+
state.setSelectedOnChange;
87+
},
88+
default: () => {
89+
state.boundSetSelectedOnChange = state.setSelectedOnChange;
90+
}
91+
});
7392
state.initialized = true;
7493
});
7594
// jscpd:ignore-end
7695

7796
onUpdate(() => {
78-
if (state.initialized && _ref) {
79-
if (props.active) {
80-
_ref.click();
81-
}
82-
97+
if (_ref && state.initialized && state.boundSetSelectedOnChange) {
8398
useTarget({ react: () => state.handleNameAttribute() });
8499
state.initialized = false;
100+
101+
// deselect this tab when another tab in tablist is selected
102+
if (!state._listenerAdded) {
103+
_ref.closest('[role=tablist]')?.addEventListener(
104+
'change',
105+
state.boundSetSelectedOnChange
106+
);
107+
state._listenerAdded = true;
108+
}
109+
110+
// Initialize selected state from either active prop (set by parent) or checked attribute
111+
if (props.active || _ref.checked) {
112+
useTarget({
113+
stencil: () => {
114+
state._selected = getBooleanAsString(true);
115+
},
116+
default: () => {
117+
state._selected = true;
118+
}
119+
});
120+
_ref.click();
121+
}
85122
}
86-
}, [_ref, state.initialized]);
123+
}, [_ref, state.initialized, state.boundSetSelectedOnChange]);
87124

88125
onUpdate(() => {
89126
if (props.name) {
90127
state._name = props.name;
91128
}
92129
}, [props.name]);
93130

131+
onUnMount(() => {
132+
if (state._listenerAdded && _ref && state.boundSetSelectedOnChange) {
133+
_ref.closest('[role=tablist]')?.removeEventListener(
134+
'change',
135+
state.boundSetSelectedOnChange
136+
);
137+
state._listenerAdded = false;
138+
}
139+
});
140+
94141
return (
95142
<li class={cls('db-tab-item', props.className)} role="none">
96143
<label

‎packages/components/src/components/tabs/tabs.lite.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,11 +170,11 @@ export default function DBTabs(props: DBTabsProps) {
170170
if (tabItem) {
171171
const list = tabItem.parentElement;
172172
if (list) {
173-
const indices = Array.from(list.childNodes).indexOf(
173+
const tabIndex = Array.from(list.children).indexOf(
174174
tabItem
175175
);
176176
if (props.onIndexChange) {
177-
props.onIndexChange(indices);
177+
props.onIndexChange(tabIndex);
178178
}
179179

180180
if (props.onTabSelect) {

‎packages/components/src/components/tabs/tabs.spec.tsx‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,34 @@
11
import AxeBuilder from '@axe-core/playwright';
22
import { expect, test } from '@playwright/experimental-ct-react';
33

4-
import { existsSync, rmSync, writeFileSync } from 'node:fs';
54
import { DBTabs } from './index';
65
// @ts-ignore - vue can only find it with .ts as file ending
76
import { DEFAULT_VIEWPORT } from '../../shared/constants.ts';
87
import { DBTabItem } from '../tab-item';
98
import { DBTabList } from '../tab-list';
109
import { DBTabPanel } from '../tab-panel';
1110

12-
const filePath = './test-results/onIndexChange.txt';
11+
let activeTabIndex: number | undefined;
12+
let comp: any = null;
1313

14-
const comp: any = (
15-
<DBTabs
16-
onIndexChange={(index: number) =>
17-
writeFileSync(filePath, index.toString())
18-
}>
19-
<DBTabList>
20-
<DBTabItem data-testid="test">Test 1</DBTabItem>
21-
<DBTabItem data-testid="test2">Test 2</DBTabItem>
22-
<DBTabItem>Test 3</DBTabItem>
23-
</DBTabList>
14+
test.beforeEach(() => {
15+
activeTabIndex = undefined;
16+
comp = (
17+
<DBTabs onIndexChange={(index: number) => (activeTabIndex = index)}>
18+
<DBTabList>
19+
<DBTabItem data-testid="test">Test 1</DBTabItem>
20+
<DBTabItem data-testid="test2">Test 2</DBTabItem>
21+
<DBTabItem>Test 3</DBTabItem>
22+
</DBTabList>
2423

25-
<DBTabPanel>TestPanel 1</DBTabPanel>
24+
<DBTabPanel>TestPanel 1</DBTabPanel>
2625

27-
<DBTabPanel>TestPanel 2</DBTabPanel>
26+
<DBTabPanel>TestPanel 2</DBTabPanel>
2827

29-
<DBTabPanel>TestPanel 3</DBTabPanel>
30-
</DBTabs>
31-
);
28+
<DBTabPanel>TestPanel 3</DBTabPanel>
29+
</DBTabs>
30+
);
31+
});
3232

3333
const testComponent = () => {
3434
test('should contain text', async ({ mount }) => {
@@ -44,10 +44,11 @@ const testComponent = () => {
4444

4545
const testActions = () => {
4646
test('should be clickable', async ({ mount }) => {
47-
if (existsSync(filePath)) {
48-
rmSync(filePath);
49-
}
47+
expect(activeTabIndex).toBe(undefined);
5048

49+
// Beware: the comments below actually change the selector for vue
50+
// this is necessary because vue will not trigger a check on an list element but requires the actual
51+
// radio button element, which has the role=tab
5152
const component = await mount(comp);
5253
await component
5354
.getByTestId('test2')
@@ -59,7 +60,7 @@ const testActions = () => {
5960
.isChecked();
6061
expect(!tabChecked).toBeTruthy();
6162

62-
expect(existsSync(filePath)).toBeTruthy();
63+
expect(activeTabIndex).toBe(1);
6364
});
6465
};
6566

0 commit comments

Comments
 (0)