fix(Popover): fixed focus trapped on hoverable trigger

[thatblindgeye]

What: Closes #10257

Additional issues:

Summary by CodeRabbit

• Bug Fixes

  • Prevented focus trapping from being enabled by default for hover-triggered popovers, aligning focus behavior with expected hover interactions.

• Documentation

  • Updated the “Hoverable” popover example to explicitly use triggerAction="hover" for hover-based behavior.
  • Added accessibility guidance warning that hoverable popovers should not include interactive/semantic content since focus is not intended to enter that area.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: af3fcad9-95d8-4b5e-aff6-07857aa6ffd8

📥 Commits

Reviewing files that changed from the base of the PR and between d2a029f and ee93b1c.

📒 Files selected for processing (2)

• packages/react-core/src/components/Popover/Popover.tsx
• packages/react-core/src/components/Popover/examples/Popover.md

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/react-core/src/components/Popover/Popover.tsx

---

Walkthrough

Popover hover triggers now disable focus trapping by default. The Hoverable example documents the required trigger configuration, content restrictions, and tooltip alternative.

Changes

Popover hover accessibility

Layer / File(s)	Summary
Disable focus trapping for hover triggers packages/react-core/src/components/Popover/Popover.tsx	withFocusTrap defaults to false when triggerAction is hover, and the prop documentation describes this behavior.
Document hoverable content constraints packages/react-core/src/components/Popover/examples/Popover.md	The Hoverable example documents triggerAction="hover", prohibits interactive or semantic content, and recommends the tooltip component.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes hover focus trapping and adds guidance, but it does not address the screen-reader announcement/aria-describedby requirement in [#10257].	Add the missing screen-reader support for hoverable Popover contents, such as exposing the popup to the trigger with aria-describedby or an equivalent accessible association.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main change: preventing focus trapping for hoverable Popover triggers.
Out of Scope Changes check	✅ Passed	The changes stay focused on hoverable Popover accessibility and documentation, with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/react-core/src/components/Popover/Popover.tsx`:
- Around line 210-212: Force the resolved focus-trap setting to false whenever
triggerAction is "hover", including when withFocusTrap is explicitly true.
Update both activation and return-focus behavior to use resolvedWithFocusTrap,
and add a regression test covering the explicit-true hover case.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 70db5de2-7d79-46c8-bcdc-372827e98811

📥 Commits

Reviewing files that changed from the base of the PR and between 8fd7fdb and d2a029f.

📒 Files selected for processing (2)

• packages/react-core/src/components/Popover/Popover.tsx
• packages/react-core/src/components/Popover/examples/Popover.md

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Force hover-triggered popovers to disable focus trapping.

This only changes the default; an explicit withFocusTrap={true} still enables the trap for triggerAction="hover", contradicting the documented “must remain false” contract and the PR objective. Normalize the resolved value to false whenever triggerAction === 'hover', and add a regression test for the explicit-true case.

Proposed direction

-  withFocusTrap: propWithFocusTrap = triggerAction === 'hover' ? false : undefined,
+  withFocusTrap: propWithFocusTrap,
...
+  const resolvedWithFocusTrap = triggerAction === 'hover' ? false : propWithFocusTrap;
-  const [focusTrapActive, setFocusTrapActive] = useState(Boolean(propWithFocusTrap));
+  const [focusTrapActive, setFocusTrapActive] = useState(Boolean(resolvedWithFocusTrap));

Use resolvedWithFocusTrap for activation and returnFocusOnDeactivate as well.

Also applies to: 272-272

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/react-core/src/components/Popover/Popover.tsx` around lines 210 -
212, Force the resolved focus-trap setting to false whenever triggerAction is
"hover", including when withFocusTrap is explicitly true. Update both activation
and return-focus behavior to use resolvedWithFocusTrap, and add a regression
test covering the explicit-true hover case.

[kmcfaul]

LGTM. Do you think Coderabbit's suggestion of enforcing the disabled focus trap for hover triggers is worth implementing? If we do, it's a little more breaking than just updating the default and I can see a user reporting it as a bug, but they shouldn't be doing it.

[thatblindgeye]

@kmcfaul personally I think it'd make sense to force it like coderabbit suggested, but updating the default for now should suffice wwith the additional verbiage. Plus if we do end up deprecating/removing hvoerable popovers.