Repository navigation
Conversation
Coverage report for library
Test suite run success3283 tests passing in 76 suites. Report generated by 🧪jest coverage report action from 476e39e |
There was a problem hiding this comment.
🟡 Changes recommended
The core fix is sound, but a misleading click-outside test should be removed, and pointer users in Safari and Firefox on macOS may still lose focus.
1 open issue
What changed in this PR
This PR fixes a focus problem in the table and tree table column filters. When a user closes the filter menu with Apply, Clear (when hideOnClear is set), or Close, focus used to fall to <body> because the focused button was removed with the menu. TableColumnFilterComponent.hide() now moves focus back to that column's filter button when focus was inside the menu. Because cps-table and cps-tree-table share this component, one change fixes both. Esc and click-outside behavior are unchanged. The PR closes #896.
Changes:
hide()checks whether focus is inside the menu. If it is, focus moves to the filter button throughCPS_FOCUS_SERVICE.focusElement(...), with abutton.focus()fallback.- The filter button in the template gets a
#filterMenuButtontemplate reference. - Jest tests cover focus return for Apply, Clear, and Close, with and without a focus ring. Playwright tests cover Apply on both table types.
| File | Description |
|---|---|
| projects/cps-ui-kit/src/lib/components/cps-table/components/internal/table-column-filter/table-column-filter.component.ts | Adds the focus check in hide() and the helper that returns focus to the filter button. |
| projects/cps-ui-kit/src/lib/components/cps-table/components/internal/table-column-filter/table-column-filter.component.spec.ts | Adds Jest tests for focus return, focus ring behavior, and focus staying outside the menu. |
| projects/cps-ui-kit/src/lib/components/cps-table/components/internal/table-column-filter/table-column-filter.component.html | Adds the #filterMenuButton reference to the filter button. |
| playwright/cps-ui-kit/components/cps-tree-table.spec.ts | Adds end-to-end checks that tree table focus returns to the filter button after Apply, with and without keyboard use. |
| playwright/cps-ui-kit/components/cps-table.spec.ts | Adds the same end-to-end checks for cps-table. |
Review effort: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The fix is small and well tested, but the PR description says focus outside the menu is left alone, while the code also returns focus when focus is on <body>, and this needs a code comment and a description update before merge.
1 open issue
1 resolved since last review
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The change alters focus handling in a shared component and in the public hideFilter() API, and the <body> case depends on browser-specific click behavior that needs human verification.
No open issues 🎉
1 resolved since last review
Review effort: Lite
Playwright test resultsDetails
|
| await expect(filterBtn).toBeFocused(); | ||
| expect( | ||
| await filterBtn.evaluate((el) => el.matches(':focus-visible')) | ||
| ).toBe(true); |
There was a problem hiding this comment.
| ).toBe(true); | |
| ).toBe(true); | |
| await expect(filterBtn).not.toHaveClass(/suppress-focus-visible/); |
nit: The test might pass even if the focus ring is not visible (:focus-visible still matches when the ring is suppressed via .suppress-focus-visible). We might want to assert suppression class not being present.
Same applies for cps-table.spec.ts:L95-97
There was a problem hiding this comment.
Good catch, thanks. Applied the suggestion
…losing-a-column-filter-with-apply-clear-or-close
…losing-a-column-filter-with-apply-clear-or-close

Summary
hideOnClear) or Close (×), focus fell to<body>, because the focused button inside the menu was removed.TableColumnFilterComponent.hide()now returns focus to that column's filter button throughCPS_FOCUS_SERVICE.focusElement(button, isKeyboard()), so the focus ring shows only after keyboard use. Without the focus service, it falls back tobutton.focus().<body>.<body>counts as inside the menu because Safari and Firefox on macOS don't focus clicked buttons, so clicking Apply, Clear or Close leaves focus on<body>there. Focus on any other element outside the menu is left alone. Esc and click-outside behave as before.hide(), so the publichideFilter()on thecpsTColFilter/cpsTTColFilterdirectives behaves the same way. Calling it on an open menu while focus is on<body>moves focus to the filter button. Calling it on a closed menu doesn't move focus.cps-tableandcps-tree-table, so this one change fixes both.Tableand once withTreeTable, open the real menu and check that:<body>;hide()leaves focus alone when it's outside the menu or when the menu is already closed;:focus-visiblematches) and by mouse (focus returns and the ring is suppressed). Verified in Chromium and WebKit. In WebKit, the mouse-click test fails without the<body>handling.Closes #896
Release notes: