feat(forms): coordinate interest and popover interactions#175
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds focus-visible gating and popover-open suppression to interest handling, refactors popover click flow into target resolution and interest handover, adds a coordinated tooltip/dropdown example and docs, and raises several Lighthouse JavaScript size thresholds. ChangesInterest handover between tooltips and popovers
Lighthouse budget updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/core/src/alert/alert.test.lighthouse.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/core/src/combobox/combobox.test.lighthouse.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. projects/core/src/copy-button/copy-button.test.lighthouse.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/forms/src/internal/controllers/type-popover-trigger.controller.ts (1)
9-17: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
InterestEventtype andinterestForElementfield duplicate thetype-interest-invoker.controller.tscontract.The same
InterestEventshape (Event & { source: HTMLElement }) and a near-identicalinterestForElementrequirement are now declared independently in bothtype-interest-invoker.controller.tsand this file, and re-declared inline again in test files (e.g.(event as Event & { source: HTMLElement })). ExtractingInterestEventinto the sharedprojects/forms/src/internal/controllers/types.ts(which already exportsReactiveElement) and importing it from both controllers/tests would prevent drift.🤖 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 `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts` around lines 9 - 17, Extract the duplicated InterestEvent contract from type-interest-invoker.controller.ts and type-popover-trigger.controller.ts into the shared types module that already exports ReactiveElement, then import and reuse that shared type in both controllers and related tests instead of re-declaring Event & { source: HTMLElement } inline. Also centralize the interestForElement requirement through the shared host shape so the two controllers stay aligned and do not drift.
🤖 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 `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts`:
- Around line 62-75: The click handler in type-popover-trigger.controller.ts
resolves and reassigns the popover target before checking whether the host is
disabled, so disabled clicks still do unnecessary work and state mutation.
Update `#onClick` to short-circuit on event.defaultPrevented and
this.host.disabled before calling `#resolvePopoverTarget`(), then keep the
existing `#invokePopover` and `#handoverInterest` flow unchanged for enabled clicks.
---
Outside diff comments:
In `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts`:
- Around line 9-17: Extract the duplicated InterestEvent contract from
type-interest-invoker.controller.ts and type-popover-trigger.controller.ts into
the shared types module that already exports ReactiveElement, then import and
reuse that shared type in both controllers and related tests instead of
re-declaring Event & { source: HTMLElement } inline. Also centralize the
interestForElement requirement through the shared host shape so the two
controllers stay aligned and do not drift.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 40291b12-983d-4aa9-9bca-494ba99e01b2
📒 Files selected for processing (8)
projects/core/src/tooltip/tooltip.examples.tsprojects/core/src/tooltip/tooltip.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.test.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.tsprojects/forms/src/mixins/button.test.tsprojects/site/src/docs/elements/tooltip.md
johnyanarella
force-pushed
the
topic/coordinate-interestfor-popovertarget
branch
from
0fb8659 to
22b8c8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/forms/src/internal/controllers/type-popover-trigger.controller.ts (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the duplicated
InterestEventtype.
InterestEventis declared identically here and intype-interest-invoker.controller.ts. Extracting it to a shared module (e.g. alongsideReactiveElementincontrollers/types.ts) would avoid drift between the two copies.🤖 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 `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts` around lines 9 - 17, The duplicated InterestEvent type should be consolidated so both controllers use the same definition instead of maintaining separate copies. Move the shared InterestEvent alias to a common module such as controllers/types.ts, then update type-popover-trigger.controller.ts and type-interest-invoker.controller.ts to import it; keep PopoverTriggerHost unchanged except for using the shared type where needed.
♻️ Duplicate comments (1)
projects/core/src/tooltip/tooltip.test.ts (1)
175-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBlanket
matchesmock same concern as in button.test.ts.
mockReturnValue(false)overrides every selector check ontrigger, not just:focus-visible, risking silent interference with any other internalmatches()usage during the click/dismiss/focus sequence.♻️ Scope mock to the selector under test
- const focusVisibleMatch = vi.spyOn(trigger, 'matches').mockReturnValue(false); + const focusVisibleMatch = vi + .spyOn(trigger, 'matches') + .mockImplementation((selector: string) => (selector === ':focus-visible' ? false : trigger.matches(selector)));🤖 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 `@projects/core/src/tooltip/tooltip.test.ts` around lines 175 - 184, The test in tooltip.test.ts is mocking trigger.matches too broadly, so it overrides every selector check instead of only :focus-visible. Update the spy in the tooltip dismissal/revive test to scope the mocked return value to the :focus-visible selector only, while preserving real behavior for any other matches() calls during emulateClick, dropdown.hidePopover, and the focus dispatch.
🤖 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 `@projects/core/src/tooltip/tooltip.examples.ts`:
- Around line 400-409: The PopoverInterest example in tooltip.examples.ts
renders bare elements, unlike the surrounding examples that use the standard
layout wrapper. Update the render markup in PopoverInterest to follow the same
example structure as the neighboring snippet by wrapping the button, tooltip,
and dropdown inside the existing section-based layout pattern used elsewhere in
this file, so the docs page stays visually consistent and aligned with the
examples convention.
In `@projects/core/src/tooltip/tooltip.test.ts`:
- Around line 127-145: The final assertion in the tooltip test is checking
`element.matches(':popover-open')` too early, so it never proves that hover
interest was actually suppressed. In `tooltip.test.ts`, update the `should hide
an open tooltip when its trigger opens an interactive popover` case around
`emulateMouseEnter(trigger)` to let the scheduled interest task run first,
matching the fake-timer pattern used in the nearby tooltip tests. Then assert
that the tooltip remains closed after that awaited/ticked step.
In
`@projects/forms/src/internal/controllers/type-popover-trigger.controller.test.ts`:
- Around line 68-84: Add a regression test in
type-popover-trigger.controller.test.ts for the disabled-host guard using
PopoverTriggerControllerTestElement: verify that when the host is disabled and
receives a click, it does not call togglePopover and does not change
popoverTargetElement. Reuse the existing test setup style around the
PopoverTriggerControllerTestElement, popover element, and click dispatch, but
assert the disabled state prevents both popover invocation and any mutation on
the controller’s target element.
---
Outside diff comments:
In `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts`:
- Around line 9-17: The duplicated InterestEvent type should be consolidated so
both controllers use the same definition instead of maintaining separate copies.
Move the shared InterestEvent alias to a common module such as
controllers/types.ts, then update type-popover-trigger.controller.ts and
type-interest-invoker.controller.ts to import it; keep PopoverTriggerHost
unchanged except for using the shared type where needed.
---
Duplicate comments:
In `@projects/core/src/tooltip/tooltip.test.ts`:
- Around line 175-184: The test in tooltip.test.ts is mocking trigger.matches
too broadly, so it overrides every selector check instead of only
:focus-visible. Update the spy in the tooltip dismissal/revive test to scope the
mocked return value to the :focus-visible selector only, while preserving real
behavior for any other matches() calls during emulateClick,
dropdown.hidePopover, and the focus dispatch.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 24ddf55d-2ae0-4f8b-a6fb-02b3b60234c6
📒 Files selected for processing (9)
projects/core/src/index.test.lighthouse.tsprojects/core/src/tooltip/tooltip.examples.tsprojects/core/src/tooltip/tooltip.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.test.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.tsprojects/forms/src/mixins/button.test.tsprojects/site/src/docs/elements/tooltip.md
johnyanarella
force-pushed
the
topic/coordinate-interestfor-popovertarget
branch
from
22b8c8d to
37e8457
Compare
There was a problem hiding this comment.
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 `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts`:
- Around line 85-99: In `#invokePopover` of TypePopoverTriggerController, guard
the direct showPopover() and hidePopover() calls against the current
:popover-open state so repeated clicks do not throw InvalidStateError and stop
the handler before `#handoverInterest`() runs. Keep togglePopover() unchanged, but
add the open/closed check around the 'show' and 'hide' branches using the
existing popoverTargetElement and host.popoverTargetAction logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 28a1bff0-9631-4f7f-9530-28e76c1a1366
📒 Files selected for processing (27)
projects/core/src/alert/alert.test.lighthouse.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/copy-button/copy-button.test.lighthouse.tsprojects/core/src/datetime/datetime.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/icon-button/icon-button.test.lighthouse.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/month/month.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/panel/panel.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/sort-button/sort-button.test.lighthouse.tsprojects/core/src/steps/steps.test.lighthouse.tsprojects/core/src/tag/tag.test.lighthouse.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tooltip/tooltip.examples.tsprojects/core/src/tooltip/tooltip.test.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/core/src/week/week.test.lighthouse.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.test.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.tsprojects/forms/src/mixins/button.test.tsprojects/site/src/docs/elements/tooltip.md
coryrylan
approved these changes
Jul 8, 2026
coryrylan
left a comment
coryrylan
left a comment
Collaborator
There was a problem hiding this comment.
approve pending commit split
Signed-off-by: John Yanarella <jyanarella@nvidia.com>
Signed-off-by: John Yanarella <jyanarella@nvidia.com>
johnyanarella
force-pushed
the
topic/coordinate-interestfor-popovertarget
branch
from
37e8457 to
726ed33
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/forms/src/internal/controllers/type-popover-trigger.controller.ts (1)
85-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
showPopover()/hidePopover()againstInvalidStateError— still unaddressed.Per the HTML spec,
showPopover()throwsInvalidStateErrorif already showing, andhidePopover()throws if already hidden.#invokePopovercalls these unconditionally for'hide'/'show'actions without checking:popover-openfirst, so a repeated click with the same action (e.g., a second "show" click while already open) would throw and abort#onClickbefore#handoverInterest()runs.togglePopover()is unaffected since it doesn't throw on repeat calls.This is the same concern raised on a previous commit of this file and does not appear to have been fixed in the current version.
🛡️ Proposed fix
`#invokePopover`(popoverTargetElement: HTMLElement) { if (this.host.popoverTargetAction === 'hide') { - popoverTargetElement.hidePopover(); + if (popoverTargetElement.matches(':popover-open')) { + popoverTargetElement.hidePopover(); + } return false; } const source = this.#getPopoverSource(popoverTargetElement); if (this.host.popoverTargetAction === 'show') { - popoverTargetElement.showPopover({ source }); + if (!popoverTargetElement.matches(':popover-open')) { + popoverTargetElement.showPopover({ source }); + } } else { popoverTargetElement.togglePopover({ source }); } return popoverTargetElement.matches(':popover-open'); }🤖 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 `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts` around lines 85 - 99, `#invokePopover` in `TypePopoverTriggerController` still calls `showPopover()` and `hidePopover()` unconditionally, which can throw `InvalidStateError` on repeated actions. Update the `'show'` and `'hide'` branches to first check whether the target matches `:popover-open` and only call `showPopover()` when closed or `hidePopover()` when open, leaving `togglePopover()` unchanged. Keep the fix localized to `#invokePopover(popoverTargetElement)` so `#onClick` can continue to `#handoverInterest()` after repeated clicks.
🤖 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.
Duplicate comments:
In `@projects/forms/src/internal/controllers/type-popover-trigger.controller.ts`:
- Around line 85-99: `#invokePopover` in `TypePopoverTriggerController` still
calls `showPopover()` and `hidePopover()` unconditionally, which can throw
`InvalidStateError` on repeated actions. Update the `'show'` and `'hide'`
branches to first check whether the target matches `:popover-open` and only call
`showPopover()` when closed or `hidePopover()` when open, leaving
`togglePopover()` unchanged. Keep the fix localized to
`#invokePopover(popoverTargetElement)` so `#onClick` can continue to
`#handoverInterest()` after repeated clicks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1c4fba43-c058-4016-b33d-564da99d7fa5
📒 Files selected for processing (27)
projects/core/src/alert/alert.test.lighthouse.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/copy-button/copy-button.test.lighthouse.tsprojects/core/src/datetime/datetime.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/icon-button/icon-button.test.lighthouse.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/month/month.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/panel/panel.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/sort-button/sort-button.test.lighthouse.tsprojects/core/src/steps/steps.test.lighthouse.tsprojects/core/src/tag/tag.test.lighthouse.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tooltip/tooltip.examples.tsprojects/core/src/tooltip/tooltip.test.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/core/src/week/week.test.lighthouse.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.test.tsprojects/forms/src/internal/controllers/type-interest-invoker.controller.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.test.tsprojects/forms/src/internal/controllers/type-popover-trigger.controller.tsprojects/forms/src/mixins/button.test.tsprojects/site/src/docs/elements/tooltip.md
Collaborator
|
🎉 This issue has been resolved in version 2.0.2 🎉 |
Collaborator
|
🎉 This issue has been resolved in version 2.0.4 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
:focus-visiblewhile preserving pointer behavior.Summary by CodeRabbit
focus-visible, and improved tooltip interest/loseinterest handover across popover open/close and prevented toggles (including anchored targets).