[JEWEL-1029] [JEWEL-1230] Reimplement Dropdown component
#3409
Conversation
wellingtoncosta
requested review from
AlexVanGogen,
DanielSouzaBertoldi,
daaria-s,
nebojsa-vuksic and
rock3r
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| adContent?.let { PopupAd(modifier = Modifier.fillMaxWidth()) { it() } } | ||
| MenuStyle(isDark = style.isDark, colors = style.colors, metrics = adjustedMetrics, icons = style.icons) | ||
| } else { | ||
| style |
There was a problem hiding this comment.
useFullWidthSelection flag has no effect on metrics
Medium Severity
The useFullWidthSelection parameter in MenuContentImpl is intended to adjust menu item metrics for full-width selection highlighting, but the adjustedItemMetrics construction simply copies all values from the original itemMetrics without any modifications. The outerPadding field (which controls the selection highlight inset) is copied unchanged, making the entire conditional branch a no-op. This affects MenuComboBox, which explicitly sets useFullWidthSelection = true expecting adjusted selection visuals.
Additional Locations (1)
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/MenuComboBox.kt
Show resolved
Hide resolved
DanielSouzaBertoldi
left a comment
DanielSouzaBertoldi
left a comment
There was a problem hiding this comment.
Awesome stuff! 🚀
Just some minor changes + a little nit-picking 😈
There was a problem hiding this comment.
I think it's best to make this distinction as early as possible in the KDoc, since the idea is to strongly suggest users to use this variant from now on, right?
| * This version of ComboBox allows for complete customization of the label area through a composable function and also includes additional parameters for controlling popup behavior, keyboard events, and width measurement. |
And also add the reason why this variant is the one users should stick with 90% of the time
There was a problem hiding this comment.
| * @param useIntrinsicPopupWidth Whether to use intrinsic width measurement for the popup. Set it to `true` when the popup | |
| * content must size itself to its widest item (e.g., `MenuContent`). Set to false (default) when the content contains | |
| * `SubcomposeLayout`-based components (`LazyColumn`, `LazyRow`, etc.) which don't support intrinsic measurements |
There was a problem hiding this comment.
In the KDoc it's stated that this parameter has a default value of false, but that's not the case in the actual code.
There was a problem hiding this comment.
Once your PR is merged, I'll have to make sure to add a variant that does not let the user set the maxPopupHeight but instead should be high enough for an arbitrary number of rows to appear at once in my PR (#3384)
There was a problem hiding this comment.
Why is the default for focusable false? Thinking as a user, I'd probably like to have the popup automatically gain focus once I click something that opens it.
Also, this property is set as true by default in the PopupMenu variant that you added
Or is there some context here I'm missing? 🤯
There was a problem hiding this comment.
Not sure if this was changed. Every other code here sets focusable as true, but this is still false. Is this expected?
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Outdated
Show resolved
Hide resolved
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Show resolved
Hide resolved
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Outdated
Show resolved
Hide resolved
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Sorry, I don't get it. If users don't set a maxHeight then the content will scroll independently from the adContent?
Or by "original behavior" you mean the fact that maybe there's no vertical scroll at all?
There was a problem hiding this comment.
I reworked this to be more intuitive rephrased these comments. Also found a subtle bug in some combo box samples regarding measurement. Things are now more clear and working.
There was a problem hiding this comment.
It's always a good idea to add tests to check if the bugs you found have been fixed for good. It also protect us in the future if the bug happens to come again after some changes 😎
wellingtoncosta
force-pushed
the
wp/reimplement-dropdown-component
branch
from
9196ed0 to
f0e4d72
Compare
|
@DanielSouzaBertoldi I resolved all your comments (I hope so), so please take a look on this PR again once you have a chance. Let me know if you have more questions or concerns. |
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Outdated
Show resolved
Hide resolved
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/Dropdown.kt
Show resolved
Hide resolved
wellingtoncosta
force-pushed
the
wp/reimplement-dropdown-component
branch
from
f0e4d72 to
5d93115
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
platform/jewel/ui/src/main/kotlin/org/jetbrains/jewel/ui/component/PopupContainer.kt
Show resolved
Hide resolved
wellingtoncosta
force-pushed
the
wp/reimplement-dropdown-component
branch
from
5d93115 to
d9be05c
Compare
wellingtoncosta
force-pushed
the
wp/reimplement-dropdown-component
branch
from
d9be05c to
96b26ca
Compare
|
@wellingtoncosta I noticed some changes are still pending. Let me know once you reply to my comments/add the changes, please |
| onPopupVisibleChange: (visible: Boolean) -> Unit = {}, | ||
| content: MenuScope.() -> Unit, | ||
| ) { | ||
| val popupManager = remember { PopupManager(onPopupVisibleChange) } |
There was a problem hiding this comment.
This code is still not keying on onPopupVisibleChange or updated to use rememberUpdatedState instead.
2 participants
Summary
This PR introduces a significant refactoring of
PopupMenuto usePopupContainerinstead of directly usingPopup, consolidating visual styling (shadows, borders, backgrounds) and scrolling logic in one place. Previously,MenuContenthad its own implementation of these features; nowPopupContainerhandles all popup styling consistently across the application, withMenuContentfocusing solely on rendering menu items. Both menu content and ad content are now in the same scrollable area withinPopupContainer.To support this refactoring,
PopupContainerwas enhanced with scrolling support viamaxHeight,useIntrinsicWidthfor width control, and key event handling. ThePopupMenuAPI now accepts bothmenuStyleandpopupContainerStyleparameters to better separate menu item styling from container styling.Additionally, this PR introduces
MenuComboBox, a modern replacement for the deprecatedDropdowncomponent that combinesComboBoxUI withMenuContentfor richer popup functionality including icons, keybindings, separators, and submenus. The component provides better keyboard navigation, focus management, and accessibility support.Showcase samples were updated to demonstrate
MenuComboBoxusage.Screen recording
Screen.Recording.2026-02-05.at.11.51.22.AM.mov
Release notes
Dropdowncomponent is now deprecated in favor ofMenuComboBox, which provides better keyboard navigation and accessibility support with automatic replacement providedNew features
PopupContainerwith optionalmaxHeightparameter and vertical scrollbaruseIntrinsicWidthparameter toPopupContainerfor controlling width measurement (set tofalseforSubcomposeLayout-based components likeLazyColumn)PopupContainerviaonPreviewKeyEventandonKeyEventparametersPopupMenuoverloads accepting bothmenuStyleandpopupContainerStylefor better separation of menu item styling from container stylingMenuComboBoxas modernDropdownreplacement with support for menu items with icons, keybindings, separators, and submenusDeprecated API
Dropdownin favor ofMenuComboBoxwithDeprecationLevel.WARNINGPopupContainer,PopupMenu, andComboBoxoverloads in favor of new variants with enhanced parametersNote
Medium Risk
Touches core popup/menu/combobox UI primitives and keyboard handling, so regressions in sizing, focus, and dismissal behavior are possible despite the changes being largely contained to UI infrastructure.
Overview
Reimplements dropdown-style menus by introducing experimental
MenuComboBox(menu-item popup with icons/separators/submenus) and deprecatingDropdownwith an autoReplaceWith.Refactors
PopupMenuto render viaPopupContainer(centralizing border/shadow/background, optional scrolling viamaxHeight, and container styling) and simplifiesMenuContentto focus on item rendering; updates call sites to the newmenuStyle/popupContainerStyleparameter split.Extends
PopupContainer(scrolling + scrollbar,useIntrinsicWidth, key event hooks) and adds an experimentalComboBoxoverload to passPopupProperties, popup key handling, and intrinsic-width control; updates the showcase sample to demonstrateMenuComboBoxand refreshes API dumps.Written by Cursor Bugbot for commit 96b26ca. This will update automatically on new commits. Configure here.