Skip to content

refactor: Keyboard focus ring controller#1684

Merged
damyanpetev merged 11 commits intomasterfrom
rkaraivanov/focus-ring-refactor
Jun 5, 2025
Merged

refactor: Keyboard focus ring controller#1684
damyanpetev merged 11 commits intomasterfrom
rkaraivanov/focus-ring-refactor

Conversation

@rkaraivanov
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member

@damyanpetev damyanpetev left a comment

Choose a reason for hiding this comment

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

Outside of the current behavior comment and we can address separately, the actual refactor LGTM

Comment on lines +330 to +336
this.addEventListener('pointerdown', () => {
this._hasInnerFocus = false;
});

this.addEventListener('keyup', () => {
this._hasInnerFocus = true;
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't these be replaced with just this._hasInnerFocus = true; on the focusin handler?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I get that's the current behavior replicating what the focus ring did, but it doesn't make sense.
See https://igniteui.github.io/igniteui-webcomponents/?path=/story/carousel--inputs-template&args=interval:3000
Where focusing an input and moving the pointer away starts changing slides, which I don't think is nice and we should consider updating that behavior.

@damyanpetev damyanpetev merged commit 2febfa6 into master Jun 5, 2025
5 checks passed
@damyanpetev damyanpetev deleted the rkaraivanov/focus-ring-refactor branch June 5, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants