Ix 3264 focus#2614
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified focus-visible handling mechanism across multiple input and select components using a new FocusVisibleController and IxFocusableComponent base class, along with comprehensive regression tests. The review feedback highlights three key areas for improvement: resolving a potential lifecycle issue in select.tsx where the focus controller is not re-initialized upon DOM re-attachment, preventing visual flickering on focusout by checking if the newly focused element is still within the host, and adding a guard clause in setFocus to prevent state leaks when a null target is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| override componentDidLoad() { | ||
| this.focusController = startFocusVisible(this.hostElement); |
There was a problem hiding this comment.
In Stencil, componentDidLoad only runs once on initial load. If the component is detached and re-attached to the DOM, disconnectedCallback is called (which destroys the focusController), but componentDidLoad is not called again. This will leave the focusController undefined and break the focus-visible behavior and programmatic focusing after re-attachment.
To fix this, we should initialize the focusController in connectedCallback instead, and ensure we call super.connectedCallback() to preserve mixin lifecycles. Additionally, use a guard flag (such as hasDisconnected) to ensure that this initialization logic is not executed twice on initial mount if it is already handled by componentDidLoad.
private hasDisconnected = false;
override connectedCallback() {
super.connectedCallback();
if (this.hasDisconnected) {
this.focusController = startFocusVisible(this.hostElement);
}
}
override componentDidLoad() {
References
- When implementing
connectedCallbackin Stencil components to handle DOM re-attachment, use a guard flag (such ashasDisconnected) to ensure that initialization logic (like registering event listeners or observers) is not executed twice on initial mount if it is already handled bycomponentWillLoadorcomponentDidLoad.
| const onFocusout = () => { | ||
| hostEl.classList.remove(IX_FOCUS_VISIBLE_ACTIVE); | ||
| }; |
There was a problem hiding this comment.
Unconditionally removing the focus-visible class on any focusout event can cause visual flickering or incorrect focus ring states when focus moves internally between elements within the same component.
Using event.relatedTarget to check if the new focused element is still a child of the host element prevents this unnecessary class removal.
| const onFocusout = () => { | |
| hostEl.classList.remove(IX_FOCUS_VISIBLE_ACTIVE); | |
| }; | |
| const onFocusout = (event: FocusEvent) => { | |
| if (event.relatedTarget && hostEl.contains(event.relatedTarget as Node)) { | |
| return; | |
| } | |
| hostEl.classList.remove(IX_FOCUS_VISIBLE_ACTIVE); | |
| }; |
| const setFocus = (target?: HTMLElement | null) => { | ||
| // Flag the next focusin as keyboard-like so IX_FOCUS_VISIBLE_ACTIVE | ||
| // is applied even though no key was pressed. | ||
| programmaticFocusPending = true; | ||
| target?.focus(); | ||
| }; |
There was a problem hiding this comment.
If setFocus is called with a null or undefined target, programmaticFocusPending is still set to true. This can leak the pending state, causing the next unrelated focus event on the host element to incorrectly receive the focus-visible class.
Adding a guard clause to return early if no target is provided prevents this state leak.
const setFocus = (target?: HTMLElement | null) => {
if (!target) {
return;
}
// Flag the next focusin as keyboard-like so IX_FOCUS_VISIBLE_ACTIVE
// is applied even though no key was pressed.
programmaticFocusPending = true;
target.focus();
};
|
2 participants
💡 What is the current behavior?
GitHub Issue Number: #
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support