-
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?
Conversation
🦋 Changeset detectedLatest commit: 85ac495 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ee7cfbe to
d1535e1
Compare
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.
Pull Request Overview
This PR fixes the internal state management of the DBTabItem component to correctly track and update the _selected state, which improves accessibility by ensuring aria-selected attributes are properly set for screen readers. The key changes include:
- Replaced file-based test state management with an in-memory variable for testing the
onIndexChangecallback - Fixed tab index calculation in
DBTabsby usingchildreninstead ofchildNodesto exclude text nodes - Added event listener logic to synchronize
_selectedstate across tabs when selection changes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/components/src/components/tabs/tabs.spec.tsx | Replaced file system operations with module-scoped variable for testing tab index changes |
| packages/components/src/components/tabs/tabs.lite.tsx | Fixed index calculation by using list.children instead of list.childNodes and renamed variable from indices to tabIndex |
| packages/components/src/components/tab-item/tab-item.lite.tsx | Added setSelectedOnChange function, event listener management, and improved _selected state tracking to fix aria-selected behavior |
| .changeset/sharp-pumas-obey.md | Added changeset documenting the bug fix for DBTabItem internal state and accessibility improvements |
Comments suppressed due to low confidence (1)
packages/components/src/components/tabs/tabs.spec.tsx:27
- The
compcomponent is defined at module scope with a closure over the module-scopedtabIndexvariable. Since this component is reused across multiple tests (lines 31, 36, 48, 65, 70), all tests will mutate the same sharedtabIndexvariable. This creates test interdependencies and can lead to flaky tests depending on execution order.
Consider creating the component inside each test that needs it, or using a factory function that returns a fresh component instance with its own state.
const comp: any = (
<DBTabs onIndexChange={(index: number) => (tabIndex = index)}>
<DBTabList>
<DBTabItem data-testid="test">Test 1</DBTabItem>
<DBTabItem data-testid="test2">Test 2</DBTabItem>
<DBTabItem>Test 3</DBTabItem>
</DBTabList>
<DBTabPanel>TestPanel 1</DBTabPanel>
<DBTabPanel>TestPanel 2</DBTabPanel>
<DBTabPanel>TestPanel 3</DBTabPanel>
</DBTabs>
);
c05f2a0 to
184735b
Compare
Set (aria-)selected state via listener on parent tablist to also react on de-selection of item.
use var instead of file to hold click event results for test duration
Now also sets aria-selected=true|false correctly which improves screen reader behaviour.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
set child active after event registered, remove event listerner on unmount
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
add more checks and guardrails to TabItem on change events
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
184735b to
1b0d3e4
Compare
Set (aria-)selected state via listener on parent tablist to also react on de-selection of item. Closes #5288
Proposed changes
Each tab item holds an internal state
_selectedto keep track of the active tab. However before this change the state function did not correctly react to a de-selection because the event was only triggered on the newly active tab.Now each item registers an event listener on the parent component and sets de/selected according to this event.
The playwright test for the tab components has also been refactored slightly to eliminate an unneccesary file write process.
Types of changes
Bugfix (non-breaking change that fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (improvements to existing components or architectural decisions)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
🔭🐙🐈 Test this branch here: https://design-system.deutschebahn.com/core-web/review/fix-tab-item-selected-state