Skip to content

popover: Apply trigger_style to the trigger container - #3245

Open
Yi-111-a wants to merge 1 commit into
longbridge:mainfrom
Yi-111-a:fix/popover-trigger-style
Open

Yi-111-a wants to merge 1 commit into
longbridge:mainfrom
Yi-111-a:fix/popover-trigger-style

Conversation

@Yi-111-a

Copy link
Copy Markdown

Fixes #3041

Problem

Popover::trigger_style stored the StyleRefinement and nothing ever read it. On current main the field has exactly four references — the declaration, the new initializer, the setter name, and the assignment — and RenderOnce for Popover::render forwards the trigger element without applying it.

A caller that sized or inset its trigger through this setter got neither:

Popover::new("styled-popover")
    .trigger_style(StyleRefinement::default().w_full().p(px(10.)))
    .trigger(Button::new("trigger").label("Open").w_full())
    .child("Popover content")

Popup measures the element it is handed to decide where to anchor the surface, and the click target is that same element, so the refinement has to land on the container the popup measures — not on the trigger element the caller already styles.

Change

crates/component/src/popover.rs — when trigger_style is set, the trigger is wrapped in a div carrying the refinement before it goes to BasePopover::trigger_with. A popover with no trigger_style is passed through untouched, so the wrapper never appears in a layout that did not ask for one.

Also adds trigger_style_is_applied_to_the_trigger_container, which asserts the anchored surface moves when the refinement is supplied: a 40px trigger puts the surface at y = 40, and the same trigger with p(px(10.)) puts it at y = 60, because the popup positions against the grown container.

Contract question for the maintainer

DropdownMenuPopover always forwards a trigger style — dropdown_menu_with_anchor clones the trigger's own refinement and passes it to Popover::trigger_style, so every .dropdown_menu(...) call site is now affected, not just explicit callers. I read that as the setter doing what the surrounding code always intended (the field doc calls it the w_full hotfix), so I wired it up rather than narrowing the scope. Two things worth a look:

  • Whether the wrapper should be applied for DropdownMenuPopover at all, or whether that call site should stop forwarding the trigger's refinement.
  • Whether the w_full case actually resolves. The wrapper's width: 100% resolves against Popup's root, which is w_auto, so it may still shrink to fit. If the w_full hotfix is the real goal, the popup root may need to carry the width too — I kept that out of this PR because it is a layout change beyond the reported defect.

Verification

  • cargo +nightly check -p gpui-component --tests — clean
  • cargo +nightly fmt -p gpui-component -- --check — clean

I could not execute the test suite in this environment: a full build of the workspace does not fit the disk available to me, so the new test is compile-verified but not run. It asserts 40 -> 60 against the existing AnchorHarness numbers in this file, which place the same 40px trigger at y = 244 with the default 0.25rem gap. If the exact offsets are off, the failure will be in that arithmetic rather than in the behavior.

Public API

No signature changes. Popover::trigger_style is unchanged; it now has an effect.

AI assistance

Written with AI assistance (OpenCode, opencode/space-bunny-free). The diagnosis came from reading the render path on main; the test follows the existing AnchorHarness pattern in this file. I checked the four references the issue lists, confirmed no PR references trigger_style, and confirmed Div is not Selectable (hence Button as the trigger).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Popover: trigger_style is ignored

1 participant