fix: Prevent button dropdown from reopening when using filtering - #4975
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4975 +/- ##
=======================================
Coverage 97.66% 97.66%
=======================================
Files 959 960 +1
Lines 31354 31366 +12
Branches 11583 11590 +7
=======================================
+ Hits 30623 30635 +12
Misses 685 685
Partials 46 46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new focus-leave early-return can keep the dropdown open when focus moves to the trigger via keyboard navigation (e.g., Shift+Tab), leaving it open while focus exits the widget.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates ButtonDropdown focus/close handling in filtering mode to prevent the dropdown from immediately reopening when the trigger is clicked to close it, and updates integration test utilities accordingly.
Changes:
- Adjust focus-leave handling for filtering mode to avoid double-toggling when clicking the trigger.
- Track the dropdown trigger via the widget root to detect focus transitions back to the trigger.
- Update browser integration tests to use a renamed trigger-click helper and add coverage for the reopen regression.
File summaries
| File | Description |
|---|---|
| src/button-dropdown/utils/use-button-dropdown.ts | Adds trigger-aware focus-leave handling in filtering mode. |
| src/button-dropdown/internal.tsx | Plumbs trigger containment detection into the hook and merges root refs. |
| src/button-dropdown/integ/button-dropdown-positioning.test.ts | Uses updated trigger-click helper to open the dropdown. |
| src/button-dropdown/integ/button-dropdown-items.test.ts | Uses updated trigger-click helper to open the dropdown. |
| src/button-dropdown/integ/button-dropdown-filtering.test.ts | Adds integration coverage for “click trigger to close should not reopen” and updates helper usage. |
| src/button-dropdown/integ/button-dropdown-expandable.test.ts | Uses updated trigger-click helper to open the dropdown. |
| src/button-dropdown/integ/button-dropdown-events.test.ts | Uses updated trigger-click helper to open the dropdown. |
| src/button-dropdown/integ/button-dropdown-disabled-reason.test.ts | Renames the page-object helper and updates usages. |
| src/integ/page-objects/button-dropdown-page.ts | Renames openDropdown() to clickTrigger() to match behavior. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // the trigger click be the single source of truth. | ||
| const { relatedTarget } = event.detail; | ||
| if (relatedTarget && isTriggerElement(relatedTarget)) { | ||
| return; | ||
| } |
| return this.getText('#testDropdownMessage'); | ||
| } | ||
| public openDropdown() { | ||
| public clickTrigger() { |
There was a problem hiding this comment.
Renamed because this method toggles the dropdown, i.e, can open or close depending on the current state.
In the new test, it is used for both opening and closing.
There was a problem hiding this comment.
🟢 Approval recommended
The logic change is narrowly scoped, aligns with the described bug, and is covered by a targeted integration regression test.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/button-dropdown/internal.tsx:148
- This comment states the trigger is the only remaining focusable inside the widget root after focus leaves the dropdown content, but in split-button mode the main action is also focusable and is included in
rootRef.contains. Rewording the comment to describe checking the widget root (trigger/main action) avoids a misleading assumption.
src/button-dropdown/utils/use-button-dropdown.ts:22 - The new
isTriggerElementcontract says it checks whether an element is (or is inside) the dropdown trigger, but the current implementation ininternal.tsxusesrootRef.contains, which matches the whole ButtonDropdown root (including e.g. split-button main action). Updating the comment here will prevent future readers from assuming the check is trigger-only.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is narrowly scoped, aligns with the described issue, and is backed by both unit and integration coverage for the affected interaction paths.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The behavioral change is targeted and backed by new unit/integration coverage, with only a minor test-strengthening nit noted.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, aligns with the existing focus-visible mechanism, and is backed by targeted unit and integration regression tests.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
| * Defaults to the main document. | ||
| */ | ||
| export function isKeyboardInteraction(element?: Element | null): boolean { | ||
| const ownerDocument = element?.ownerDocument ?? (typeof document !== 'undefined' ? document : undefined); |
There was a problem hiding this comment.
Isn't this basically the same with const ownerDocument = element?.ownerDocument ?? document;?
Description
Issue:
AWSUI-62261How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.