fix: Fix focus on ng-select sorting element by sp94sap · Pull Request #21509 · SAP/spartacus

sp94sap · 2026-05-22T08:33:56Z

Ticket: https://jira.tools.sap/browse/CXSPA-12686
Ticket: https://jira.tools.sap/browse/CXSPA-12776

Global fix for focusing sorting component.

github-actions · 2026-05-22T08:34:14Z

🚨 PR Title Validation Failed 🚨

Your pull request title does not follow the required format. Please update it to match the expected pattern:

Expected format:
<type>: <subject>

Allowed Types

docs: Changes to documentation only
feat: New feature work
fix: Bug fixes
perf: Code improvements for performance
refactor: Code changes that are not bug fixes or features
style: Code style changes (e.g., whitespace, formatting)
test: Adding or updating tests
chore: Build, CI, scripts, configs, etc.

Example of a valid PR title

✅ feat: Add user authentication
✅ fix: Resolve checkout bug
✅ docs: Update API documentation

❌ Merge is blocked until the PR title is corrected.

Zeyber

It works. But there seems to be a lot of unrelated changes. Could you please explain why we have modified different focus directives? Are we using all of them?

There also seems to be case-specific code (ie. reference to ng-select and combobox). These directives should stay generic. We can modify sorting component itself or look into the cxNgSelectA11y directive for this specific component. I would say these places would be more appropriate for these targetted changes.

sp94sap · 2026-05-28T12:03:07Z

It works. But there seems to be a lot of unrelated changes. Could you please explain why we have modified different focus directives? Are we using all of them?

There also seems to be case-specific code (ie. reference to ng-select and combobox). These directives should stay generic. We can modify sorting component itself or look into the cxNgSelectA11y directive for this specific component. I would say these places would be more appropriate for these targetted changes.

@Zeyber
Fair point — let me clarify per directive:

PersistFocusDirective — both additions (focusTargetSelector, clearOnRestore) are used by the new [cxFocus] config in sorting.component.html:
- focusTargetSelector: '[role=combobox]' — needed because is the host, but the actual focusable element is the inner combobox . Without delegating, restoring focus to puts focus on a non-interactive wrapper.
  *clearOnRestore: true — needed because focusTargetSelector delegation does not re-trigger a focus event on the host, so the persisted key would otherwise leak across navigations and steal focus on a fresh route.

The API itself stays generic — it accepts any CSS selector and a boolean. The JSDoc mentions only as an example of a wrapper-with-inner-interactive-element pattern.

VisibleFocusDirective — agreed, the role="combobox" check did not belong in a generic directive. I've reverted that change and moved the logic to NgSelectA11yDirective, which now listens to keydown on its host and clears the mouse-focus class from the closest ancestor that has it (deferred via microtask so it runs after VisibleFocusDirective's bubbled handler). This keeps the case-specific knowledge ("ng-select keystrokes are dropdown navigation, not form filling") in the case-specific directive.

Tests added to ng-select-a11y.directive.spec.ts; the combobox-specific tests in visible-focus.directive.spec.ts were removed. The a11yConsentManagementFocusPreservation (checkbox/radio) tests stay — they cover pre-existing logic in the directive.

Let me know if you'd like the PersistFocusDirective API renamed or rescoped further.

cypress · 2026-05-28T13:56:09Z

spartacus Run #53479

Run Properties: Passed #53479 • 2a14fff153 ℹ️: Merge 5f5bb49fe3cdec4ad3f4b57e1714a4572dbe5494 into 228db493389de95fba380c5157a3...

Project	`spartacus`
Branch Review	`bugfix/CXSPA-12686-keep-sorting-focused-after-sort`
Run status	`Passed #53479`
Run duration	`04m 47s`
Commit	`2a14fff153 ℹ️: Merge 5f5bb49fe3cdec4ad3f4b57e1714a4572dbe5494 into 228db493389de95fba380c5157a3...`
Committer	`sebastianpaduch`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`3`
Pending	`0`
Skipped	`0`
Passing	`101`
View all changes introduced in this branch ↗︎

Zeyber

I think this fix is much too complicated. I've created an example PR to show all I found needed to resolve the problem using ViewChild and requestAnimationFrame. Note that using requestAnimationFrame is favourable over setTimeout as requesting the frame itself is more verbose.
#21566

Zeyber

Can we clean up any unnecessary artifacts in this PR? Lots of redundant code.

Zeyber · 2026-06-01T09:23:07Z

I see the issue persists in Coupons page with the example I shared due to rerendering happening on that page. I only tested PLP before so lets disregard my last review comment for now while I look into it.

Zeyber

Tested on MyCoupons page and now understand the reason for these changes much better. focusTargetSelector is very useful to have and the solution is working well! Feature flags also handled well. Remaining comments are minor.

sp94sap · 2026-06-01T10:56:56Z

I needed to revert the ngFocus logic because of a specific corner case. Currently, when there is only one coupon on the page and you change the sorting via Tab + Enter, the focus is lost after the second attempt. To fix this, I changed the focusing approach: instead of invoking focus() from NgSelectComponent, I apply native focus directly to the specific DOM element.
This solves the issue.

github-actions · 2026-06-01T12:47:19Z

Merge Checks Failed

Please push a commit to re-trigger the build.
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

Zeyber

It seems like there are a lot of mechanics built into the ng-select component which makes it more difficult to manage as this library is 3rd-party and we need to work around a lot of this behaviour. We also have a legacy of our on focus management tools on tops of this which adds to the complexity.

Great job on resolving the issues around this component and showing me some very unexpected edge cases!

github-actions · 2026-06-02T08:28:11Z

E2E Tests Failed ❌

📸 Download Screenshots

Commit: 6c4fbfb

github-actions · 2026-06-02T08:28:19Z

Merge Checks Failed

Please push a commit to re-trigger the build.
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

- PersistFocusDirective: add `focusTargetSelector` (delegate restore to a descendant) and `clearOnRestore` (prevent key leaking across routes). - VisibleFocusDirective: keep focus outline on `role="combobox"` keys and on Space/Enter for checkbox/radio (behind feature flag). - SortingComponent: opt into the new flags and redirect focus to inner combobox after sort for the not-remounted case (PLP). - Tests for both directives.

…yDirective Address review: keep VisibleFocusDirective generic. The case-specific "ng-select keystrokes are dropdown navigation" handling now lives in NgSelectA11yDirective, where the ng-select context belongs. - VisibleFocusDirective: revert the role="combobox" check; the directive no longer references ng-select-specific concepts. - NgSelectA11yDirective: add `onKeyDown` host listener that clears the `mouse-focus` class from the closest ancestor (microtask-deferred so it runs after VisibleFocusDirective's bubbled handler). - Tests moved accordingly.

github-actions · 2026-06-02T09:34:12Z

E2E Tests Failed ❌

📸 Download Screenshots

Commit: 5335d3a

github-actions · 2026-06-02T09:34:18Z

Merge Checks Failed

Please push a commit to re-trigger the build.
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

sp94sap requested a review from a team as a code owner May 22, 2026 08:33

github-actions Bot marked this pull request as draft May 22, 2026 08:34

sp94sap changed the title ~~bugfix(CXSPA-12686): keep sorting select focused after sort~~ fix(CXSPA-12686): keep sorting select focused after sort May 22, 2026

sp94sap changed the title ~~fix(CXSPA-12686): keep sorting select focused after sort~~ fix: keep sorting select focused after sort May 22, 2026

sp94sap changed the title ~~fix: keep sorting select focused after sort~~ fix: Keep sorting select focused after sort May 22, 2026

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch 3 times, most recently from 7ef25ee to 251693a Compare May 25, 2026 09:35

sp94sap changed the title ~~fix: Keep sorting select focused after sort~~ fix: Fix focus on ng-select sorting element May 25, 2026

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch 4 times, most recently from c71bb6c to 50b36c8 Compare May 25, 2026 14:31

sp94sap marked this pull request as ready for review May 26, 2026 08:40

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch from 50b36c8 to be6eb8f Compare May 26, 2026 11:19

Zeyber suggested changes May 28, 2026

View reviewed changes

github-actions Bot marked this pull request as draft May 28, 2026 12:03

sp94sap marked this pull request as ready for review May 28, 2026 13:37

github-actions Bot marked this pull request as draft May 29, 2026 11:34

sp94sap marked this pull request as ready for review May 29, 2026 12:37

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch from 560d935 to b24b2f0 Compare May 29, 2026 13:56

github-actions Bot marked this pull request as draft May 29, 2026 13:56

Zeyber reviewed May 29, 2026

View reviewed changes

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch from b24b2f0 to ff1a9b4 Compare June 1, 2026 08:21

sp94sap marked this pull request as ready for review June 1, 2026 08:35

Zeyber suggested changes Jun 1, 2026

View reviewed changes

Comment thread core-libs/storefront/shared/components/ng-select-a11y/ng-select-a11y.directive.ts

Comment thread core-libs/storefront/layout/a11y/keyboard-focus/persist/persist-focus.directive.ts

Comment thread core-libs/storefront/layout/a11y/keyboard-focus/keyboard-focus.model.ts

github-actions Bot marked this pull request as draft June 1, 2026 11:27

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch from 5dc1ba2 to 0756a10 Compare June 1, 2026 12:43

sp94sap marked this pull request as ready for review June 1, 2026 12:46

github-actions Bot marked this pull request as draft June 1, 2026 12:47

sp94sap marked this pull request as ready for review June 1, 2026 12:50

Zeyber approved these changes Jun 2, 2026

View reviewed changes

github-actions Bot marked this pull request as draft June 2, 2026 07:40

sp94sap marked this pull request as ready for review June 2, 2026 07:53

sebastianpaduch added 6 commits June 2, 2026 10:51

bugfix(CXSPA-12792): change private to protected

3db8cb2

bugfix(CXSPA-12686): Wrap changes in feature toggles

c417a16

bugfix(CXSPA-12763): Replace setTimeout with requestAnimationFrame

6f3a3d7

bugfix(CXSPA-12763): Pr changes

af1e990

sp94sap force-pushed the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch from 62320e9 to af1e990 Compare June 2, 2026 08:51

github-actions Bot marked this pull request as draft June 2, 2026 08:51

sp94sap marked this pull request as ready for review June 2, 2026 08:57

Empty commit

5f5bb49

github-actions Bot marked this pull request as draft June 2, 2026 10:17

sp94sap marked this pull request as ready for review June 2, 2026 11:14

sp94sap merged commit 593751d into develop Jun 2, 2026
44 checks passed

sp94sap deleted the bugfix/CXSPA-12686-keep-sorting-focused-after-sort branch June 2, 2026 11:37

Uh oh!

Conversation

sp94sap commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 22, 2026

🚨 PR Title Validation Failed 🚨

Allowed Types

Example of a valid PR title

Uh oh!

Zeyber left a comment

Choose a reason for hiding this comment

Uh oh!

sp94sap commented May 28, 2026

Uh oh!

cypress Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

spartacus Run #53479

Uh oh!

Zeyber left a comment

Choose a reason for hiding this comment

Uh oh!

Zeyber left a comment

Choose a reason for hiding this comment

Uh oh!

Zeyber commented Jun 1, 2026

Uh oh!

Zeyber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sp94sap commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 1, 2026

Merge Checks Failed

Uh oh!

Zeyber left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 2, 2026

E2E Tests Failed ❌

Uh oh!

github-actions Bot commented Jun 2, 2026

Merge Checks Failed

Uh oh!

github-actions Bot commented Jun 2, 2026

E2E Tests Failed ❌

Uh oh!

github-actions Bot commented Jun 2, 2026

Merge Checks Failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sp94sap commented May 22, 2026 •

edited

Loading

cypress Bot commented May 28, 2026 •

edited

Loading

sp94sap commented Jun 1, 2026 •

edited

Loading