Refactor focus management in FocusElements using callback refs#9852
Refactor focus management in FocusElements using callback refs#9852
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the focus management within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
🚀 MCP preview deployed to Vercel: https://kolibri-1uap4dkfn-public-ui-kolibri-mcp.vercel.app |
There was a problem hiding this comment.
Code Review
This pull request refactors the FocusElements component by replacing useLayoutEffect and useRef with a useCallback ref for handling focus on dynamically rendered components. The review highlights a potential issue where the setTimeout within the focusRef callback is not properly cleaned up, which could lead to memory leaks or attempts to focus unmounted elements. It is suggested to use useRef to manage and clear the timeout.
|
Netlify Draft Deployment |
c48baeb to
b2b6dc4
Compare
f6f734a to
bc7c7a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR broadens and refactors focus handling across the KoliBri component library and its sample/visual-test setup: it updates the React “FocusElements” scenario, introduces a shared delegateFocus helper for Stencil components, and adjusts several components’ focus() implementations (plus associated tests and snapshots) to make focus behavior more reliable in automated testing.
Changes:
- Refactors sample app focus scenario to use callback refs and adds additional components (badge, popoverButton, skipNav, splitButton, tabs, toolbar, tree) to the focus test matrix.
- Adds
delegateFocus()utility and updates many components’focus()methods to use/participate in the new focus delegation approach (incl. tree focus behavior and new focus-related E2E tests). - Updates visual-test routes and adds/updates theme snapshots for the newly covered focus scenarios.
Reviewed changes
Copilot reviewed 47 out of 61 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/tools/visual-tests/tests/sample-app.routes.js | Adds visual-test routes for additional focus-elements scenarios. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-tree-firefox-linux.png | New kern theme snapshot for tree focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-toolbar-firefox-linux.png | New kern theme snapshot for toolbar focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-tabs-firefox-linux.png | New kern theme snapshot for tabs focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-splitButton-firefox-linux.png | New kern theme snapshot for splitButton focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-skipNav-firefox-linux.png | New kern theme snapshot for skipNav focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-popoverButton-firefox-linux.png | New kern theme snapshot for popoverButton focus scenario. |
| packages/themes/kern/snapshots/theme-kern_v2/snapshot-for-scenarios-focus-elements-component-badge-firefox-linux.png | New kern theme snapshot for badge focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-tree-firefox-linux.png | New default theme snapshot for tree focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-toolbar-firefox-linux.png | New default theme snapshot for toolbar focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-tabs-firefox-linux.png | New default theme snapshot for tabs focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-splitButton-firefox-linux.png | New default theme snapshot for splitButton focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-skipNav-firefox-linux.png | New default theme snapshot for skipNav focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-popoverButton-firefox-linux.png | New default theme snapshot for popoverButton focus scenario. |
| packages/themes/default/snapshots/theme-default/snapshot-for-scenarios-focus-elements-component-badge-firefox-linux.png | New default theme snapshot for badge focus scenario. |
| packages/samples/react/src/scenarios/focus-elements.tsx | Refactors focus handling using callback refs; expands tested components list. |
| packages/components/src/utils/element-focus.ts | Introduces delegateFocus() helper for themed/ready focus delegation. |
| packages/components/src/functional-components/Button/tests/snapshot.test.tsx | Updates button focus snapshot test to use new focus behavior/signature. |
| packages/components/src/components/tree/tree.e2e.ts | Adds tree focus E2E test validating focus lands on first item. |
| packages/components/src/components/tree/test/snapshots/snapshot.spec.tsx.snap | Updates tree snapshot to reflect tabindex changes. |
| packages/components/src/components/tree/shadow.tsx | Adds focus method to kol-tree delegating focus through theming readiness. |
| packages/components/src/components/tree/component.tsx | Adds focus behavior/caching changes and tabindex for kol-tree-wc. |
| packages/components/src/components/tree-item/component.tsx | Updates tree-item focus interactions and cache invalidation hooks. |
| packages/components/src/components/tooltip/component.tsx | Minor typing change (readonly host). |
| packages/components/src/components/toolbar/toolbar.e2e.ts | Adds toolbar focus() method E2E coverage. |
| packages/components/src/components/toolbar/shadow.tsx | Adds focus() method for toolbar to focus current active item. |
| packages/components/src/components/textarea/shadow.tsx | Switches textarea focus() to use delegateFocus(). |
| packages/components/src/components/tabs/shadow.tsx | Implements focus() for tabs (focus current selected tab button). |
| packages/components/src/components/table-stateless/test/snapshot.spec.tsx | Updates snapshot test to new renamed WC class. |
| packages/components/src/components/table-stateless/component.tsx | Renames WC class to KolTableStatelessWc. |
| packages/components/src/components/split-button/shadow.tsx | Updates split-button focus() to delegate via host. |
| packages/components/src/components/skip-nav/shadow.tsx | Updates skip-nav focus() to delegate via host. |
| packages/components/src/components/single-select/shadow.tsx | Switches single-select focus() to use delegateFocus(). |
| packages/components/src/components/select/shadow.tsx | Updates select focus() to delegate to WC focus with host. |
| packages/components/src/components/select/component.tsx | Changes select-wc focus signature to use delegateFocus(host, ...). |
| packages/components/src/components/popover-button/shadow.tsx | Updates popover-button focus() delegation via host. |
| packages/components/src/components/popover-button/component.tsx | Renames WC class and updates focus signature to accept host. |
| packages/components/src/components/link/shadow.tsx | Updates link focus() to delegate via host. |
| packages/components/src/components/link/component.tsx | Changes link-wc focus signature to use delegateFocus(host, ...). |
| packages/components/src/components/link-button/shadow.tsx | Updates link-button focus() delegation via host. |
| packages/components/src/components/input-text/shadow.tsx | Switches input-text focus() to use delegateFocus(). |
| packages/components/src/components/input-range/shadow.tsx | Switches input-range focus() to use delegateFocus(). |
| packages/components/src/components/input-radio/shadow.tsx | Switches input-radio focus() to use delegateFocus(). |
| packages/components/src/components/input-password/shadow.tsx | Switches input-password focus() to use delegateFocus(). |
| packages/components/src/components/input-number/shadow.tsx | Switches input-number focus() to use delegateFocus(). |
| packages/components/src/components/input-file/shadow.tsx | Switches input-file focus() to use delegateFocus(). |
| packages/components/src/components/input-email/shadow.tsx | Switches input-email focus() to use delegateFocus(). |
| packages/components/src/components/input-date/shadow.tsx | Switches input-date focus() to use delegateFocus(). |
| packages/components/src/components/input-color/shadow.tsx | Switches input-color focus() to use delegateFocus(). |
| packages/components/src/components/input-checkbox/shadow.tsx | Switches input-checkbox focus() to use delegateFocus() and adjusts keydown wiring. |
| packages/components/src/components/input-checkbox/input-checkbox.e2e.ts | Adds E2E coverage for focus() and focus delegation behavior. |
| packages/components/src/components/form/shadow.tsx | Fixes host typing; adjusts first-link ref typing/cast. |
| packages/components/src/components/details/shadow.tsx | Updates details focus() to delegate via host. |
| packages/components/src/components/combobox/shadow.tsx | Switches combobox focus() to use delegateFocus(). |
| packages/components/src/components/button/shadow.tsx | Updates button focus() to delegate via host. |
| packages/components/src/components/button/component.tsx | Changes button-wc focus signature to use delegateFocus(host, ...) and adjusts event dispatch typing. |
| packages/components/src/components/button-link/shadow.tsx | Updates button-link focus() to delegate via host. |
| packages/components/src/components/badge/shadow.tsx | Updates badge focus() to delegate via host. |
| packages/components/src/components/alert/component.tsx | Minor event dispatch typing simplification. |
| packages/components/src/components/accordion/shadow.tsx | Updates accordion focus() to delegate via host. |
| packages/adapters/hydrate/test/snapshots/components.spec.js.mocha-snapshot | Updates hydrate snapshots to reflect tree tabindex changes. |
c4dd31e to
78a6915
Compare
- Added focus method to KolTreeWc to set focus on the first focusable tree item. - Implemented cache invalidation for open tree items to improve performance. - Introduced requestAnimationFrame for debouncing tree change handling. - Updated KolTree to delegate focus to KolTreeWc and handle focus events. - Enhanced keyboard navigation for tree items to improve user experience. - Added end-to-end tests for focus management and performance stability. - Updated snapshots for various components to reflect new focus behavior. - Introduced utility function for delegating focus to ensure elements are ready.
78a6915 to
90334b2
Compare
8a41354 to
c9aab5a
Compare
ee9a0d5 to
18b7a60
Compare
802dbfe to
2a3e4ee
Compare
Summary
Refactored the focus management logic in the
FocusElementscomponent to use a callback ref instead ofuseLayoutEffectwithuseRef. This improves the reliability of focus handling by ensuring focus is set whenever a component instance mounts.Key Changes
useRefanduseLayoutEffectwith auseCallbackref that fires on component mountElementcomponent type usinguseMemoto prevent unnecessary unmount/remount cycles during re-rendersuseLayoutEffect,useRef) and addeduseCallbackImplementation Details
focusRef) now directly receives the component instance when it mounts, eliminating the need for a separate effect hookElementcomponent ensures it maintains referential equality across re-renders unless the underlyingComponentchanges, preventing unnecessary component lifecycle cycleshttps://claude.ai/code/session_01KMmPfsTZpdvhQ3BGDgp39M