fix(autocomplete): focus behaviour #2854
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 99d4e1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes made across various components aim to enhance focus management and interaction behaviors in UI components like Autocomplete and Modal. These adjustments include refining focus handling on interactions, updating popover content rendering logic, and introducing new props to provide more precise control over focus behavior. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- .changeset/good-crabs-clap.md (1 hunks)
- packages/components/autocomplete/tests/autocomplete.test.tsx (4 hunks)
- packages/components/autocomplete/src/autocomplete.tsx (2 hunks)
- packages/components/autocomplete/src/use-autocomplete.ts (8 hunks)
- packages/components/modal/src/modal.tsx (1 hunks)
- packages/components/popover/src/free-solo-popover.tsx (3 hunks)
- packages/components/popover/src/use-aria-popover.ts (1 hunks)
- packages/components/select/src/select.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- .changeset/good-crabs-clap.md
- packages/components/popover/src/use-aria-popover.ts
Additional comments not posted (8)
packages/components/modal/src/modal.tsx (1)
21-21: Ensure that thedisableFocusManagementprop is properly handled in all relevant contexts.Verification successful
The
disableFocusManagementprop is indeed used across various components in the codebase, includingPopover,Modal, and within theuseAutocompletehook. This confirms that the prop is handled in multiple contexts, which aligns with the initial review comment's concern about ensuring proper handling in all relevant contexts.
- In
free-solo-popover.tsx, the prop is declared, given a default value, and passed to theOverlaycomponent.- In
modal.tsx, the prop is directly used in theOverlaycomponent without passing a specific value, implying it defaults totrue.- In
use-autocomplete.ts, the prop is set totrueexplicitly.This usage suggests that the prop's handling is consistent with its intended functionality across different components, although the default behavior and explicit settings vary slightly, which is typical given different component requirements.
Conclusion:
ThedisableFocusManagementprop is consistently used and handled across the codebase in components that integrate theOverlaycomponent or similar functionalities. The review comment is verified as the code correctly implements the functionality as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `disableFocusManagement` prop is used consistently across the codebase. # Test: Search for the usage of `disableFocusManagement` prop. Expect: Consistent usage across components. rg --type typescript $'disableFocusManagement'Length of output: 80
Script:
#!/bin/bash # Corrected script to verify the usage of `disableFocusManagement` prop across TypeScript files. # Search for the usage of `disableFocusManagement` prop in '.ts' and '.tsx' files. rg 'disableFocusManagement' --glob '*.ts' --glob '*.tsx'Length of output: 648
packages/components/autocomplete/src/autocomplete.tsx (1)
35-35: Ensure that theFreeSoloPopovercomponent receives all necessary props after the removal ofstate.Verification successful
The verification process has confirmed that the
FreeSoloPopovercomponent is used in theAutocompletecomponent without any reliance on astateprop directly passed to it. The component receives its properties through thegetPopoverProps()function, which does not includestateas a direct prop. This aligns with the changes mentioned in the review comment.
- The usage of
FreeSoloPopoverinpackages/components/autocomplete/src/autocomplete.tsxcorrectly receives properties throughgetPopoverProps()without any directstateprop.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `FreeSoloPopover` component functions correctly without the `state` prop. # Test: Search for the usage of `FreeSoloPopover` in the context of `Autocomplete`. Expect: No reliance on `state` prop. rg --type typescript $'FreeSoloPopover' -A 10 -B 10Length of output: 85
Script:
#!/bin/bash # Description: Verify that the `FreeSoloPopover` component functions correctly without the `state` prop. # Test: Search for the usage of `FreeSoloPopover` in the context of `Autocomplete`. Expect: No reliance on `state` prop. rg --type ts 'FreeSoloPopover' -A 10 -B 10Length of output: 24576
packages/components/popover/src/free-solo-popover.tsx (2)
26-26: Ensure that thedisableFocusManagementprop is properly documented and its default value is justified.
133-133: Review the usage ofdisableFocusManagementin theOverlaycomponent to ensure it aligns with the intended focus behavior.Verification successful
The search results from the script have successfully located the usage of the
Overlaycomponent with thedisableFocusManagementproperty in various parts of the codebase, including the specific instance in thefree-solo-popover.tsxfile. This confirms that the property is actively being used as intended across different components.Given this information, the usage of
disableFocusManagementin theOverlaycomponent aligns with the intended focus behavior as it is consistently applied where needed. Therefore, the original review comment has been verified as accurate.
- File:
packages/components/popover/src/free-solo-popover.tsx- Usage:
<Overlay disableFocusManagement={disableFocusManagement} portalContainer={portalContainer}>Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `Overlay` component's focus behavior aligns with the intended usage when `disableFocusManagement` is set. # Test: Search for the usage of `Overlay` with `disableFocusManagement`. Expect: Correct behavior as intended. rg --type typescript $'Overlay' -A 5 -B 5Length of output: 75
Script:
#!/bin/bash # Description: Verify that the `Overlay` component's focus behavior aligns with the intended usage when `disableFocusManagement` is set. # Test: Search for the usage of `Overlay` with `disableFocusManagement`. Expect: Correct behavior as intended. rg --type ts 'Overlay' -A 5 -B 5Length of output: 45494
packages/components/select/src/select.tsx (1)
108-108: Ensure that theFreeSoloPopovercomponent receives all necessary props after the changes in popover rendering logic.Verification successful
The verification process has confirmed that the
FreeSoloPopovercomponent is being used correctly within theSelectcomponent. The component is receiving all necessary props as indicated by the usage of{...getPopoverProps()}in theSelectcomponent's rendering logic. This pattern is consistent across other components as well, suggesting a standardized approach to handling popover properties.
- Location:
packages/components/select/src/select.tsx- Usage:
<FreeSoloPopover {...getPopoverProps()}>This confirms that the integration of the
FreeSoloPopovercomponent within theSelectcomponent adheres to the expected implementation pattern, receiving all necessary properties to function correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `FreeSoloPopover` component functions correctly with the new popover rendering logic. # Test: Search for the usage of `FreeSoloPopover` in the context of `Select`. Expect: Correct functioning with new logic. rg --type typescript $'FreeSoloPopover' -A 10 -B 10Length of output: 85
Script:
#!/bin/bash # Correcting the file type and re-running the search for `FreeSoloPopover` usage in TypeScript files. rg 'FreeSoloPopover' -A 10 -B 10 --type tsLength of output: 24576
packages/components/autocomplete/__tests__/autocomplete.test.tsx (1)
140-167: Verify that the focus behavior tests align with the new focus management logic introduced in theAutocompletecomponent.Verification successful
The search results from the script have provided relevant information regarding the focus management logic in the
Autocompletecomponent. Specifically, the output frompackages/components/autocomplete/src/use-autocomplete.tsindicates that the component explicitly disables the default focus management behavior (disableFocusManagement: true) and manages focus on its own.This information is crucial because it confirms that the
Autocompletecomponent has a custom focus management implementation. The test snippet provided in the review comment checks if the component is focused after interaction, which is consistent with the component managing its own focus as indicated by thedisableFocusManagement: truesetting.Given this alignment between the test behavior and the component's focus management logic, the focus behavior tests are verified to be consistent with the new focus management logic introduced in the
Autocompletecomponent.
- The test checks if the component is focused after being clicked, which is a valid scenario to test given the custom focus management.
- The component's logic to manage focus on its own side supports the necessity of such a test.
Therefore, the verification of the focus behavior tests with the new focus management logic in the
Autocompletecomponent is successful.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the focus behavior tests are consistent with the new focus management logic. # Test: Search for the focus management logic in the `Autocomplete` component. Expect: Consistency with tests. rg --type typescript $'focus management' -A 10 -B 10Length of output: 88
Script:
#!/bin/bash # Corrected script to search for the focus management logic in the `Autocomplete` component. # Using 'ts' as the recognized file type for TypeScript files in the ripgrep command. rg --type ts 'focus management' -A 10 -B 10Length of output: 3588
packages/components/autocomplete/src/use-autocomplete.ts (2)
202-202: Ensure that the introduction ofshouldFocusstate is well-integrated and does not conflict with existing focus management logic.Verification successful
The search results from the
rgcommand have provided us with the necessary context to assess the integration of theshouldFocusstate within theuseAutocompletehook. The output shows several instances whereshouldFocusis used in theuse-autocomplete.tsfile, particularly in relation to focus management logic.From the provided snippets, it is evident that
shouldFocusis used to conditionally focus or blur the input element based on its state and the state ofisOpen. This is a common pattern in UI development where focus management needs to be explicitly handled to enhance user experience and accessibility. The use ofshouldFocusin this manner does not appear to conflict with existing focus management logic but rather complements it by providing a controlled way to manage focus based on component state.Given this analysis, the introduction of the
shouldFocusstate seems to be well-integrated and functioning as intended without conflicts with existing focus management logic.
- The
shouldFocusstate is used to conditionally focus the input element based on its state and other conditions.- The logic surrounding
shouldFocusincludes checks and balances that ensure it does not interfere with other focus-related behaviors but enhances the component's interactivity and accessibility.Based on the evidence from the code snippets provided by the script output, the integration of the
shouldFocusstate in theuseAutocompletehook is verified to be correct and functioning as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `shouldFocus` state is integrated correctly and does not conflict with existing logic. # Test: Search for the usage of `shouldFocus` in the context of focus management. Expect: No conflicts. rg --type typescript $'shouldFocus' -A 10 -B 10Length of output: 81
Script:
#!/bin/bash # Description: Verify that the `shouldFocus` state is integrated correctly and does not conflict with existing logic. # Test: Search for the usage of `shouldFocus` in the context of focus management. Expect: No conflicts. rg 'shouldFocus' -A 10 -B 10Length of output: 23016
283-289: Review the new focus behavior on selector button click to ensure it aligns with the intended user experience.
|
This is a relatively large interaction, can this pr be merged and release? Thanks @jrgarciadev |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/autocomplete/src/use-autocomplete.ts (8 hunks)
- packages/components/popover/src/free-solo-popover.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/components/autocomplete/src/use-autocomplete.ts
- packages/components/popover/src/free-solo-popover.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- .changeset/good-crabs-clap.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .changeset/good-crabs-clap.md
📝 Description
In react aria, popover can be closed by clicking or interacting outside or by escape key (handled by useOverlay). When the popover is closed, focus is back to the trigger, i.e. the input. Since we have different focus behaviours in autocomplete, this PR is to handle them on our side instead of doing them internally on react aria side for better flexibilty.
here's the updated focus behaviour for autocomplete.
the behaviours are consistent even the autocomplete is in the modal.
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
New Features
AutocompleteandPopovercomponents.Selectcomponent by simplifying popover rendering logic.Bug Fixes
Modalcomponent by incorporatingdisableFocusManagementproperty to better control focus behavior.Refactor
Autocompletecomponent to cover new functionality scenarios.use-autocompletelogic for better focus management and interaction handling.Tests
Autocompletecomponent to ensure robustness and reliability.