Conversation
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
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.
Fixes #3041
Problem
Popover::trigger_stylestored theStyleRefinementand nothing ever read it. On currentmainthe field has exactly four references — the declaration, thenewinitializer, the setter name, and the assignment — andRenderOnce for Popover::renderforwards the trigger element without applying it.A caller that sized or inset its trigger through this setter got neither:
Popupmeasures 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— whentrigger_styleis set, the trigger is wrapped in adivcarrying the refinement before it goes toBasePopover::trigger_with. A popover with notrigger_styleis 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 aty = 40, and the same trigger withp(px(10.))puts it aty = 60, because the popup positions against the grown container.Contract question for the maintainer
DropdownMenuPopoveralways forwards a trigger style —dropdown_menu_with_anchorclones the trigger's own refinement and passes it toPopover::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 thew_fullhotfix), so I wired it up rather than narrowing the scope. Two things worth a look:DropdownMenuPopoverat all, or whether that call site should stop forwarding the trigger's refinement.w_fullcase actually resolves. The wrapper'swidth: 100%resolves againstPopup's root, which isw_auto, so it may still shrink to fit. If thew_fullhotfix 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— cleancargo +nightly fmt -p gpui-component -- --check— cleanI 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
AnchorHarnessnumbers in this file, which place the same 40px trigger aty = 244with 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_styleis 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 onmain; the test follows the existingAnchorHarnesspattern in this file. I checked the four references the issue lists, confirmed no PR referencestrigger_style, and confirmedDivis notSelectable(henceButtonas the trigger).