From fdaf71b50f8d1813b58e47ec4461b7ff786c47eb Mon Sep 17 00:00:00 2001 From: reuk Date: Tue, 19 Aug 2025 11:56:07 +0100 Subject: [PATCH] PopupMenu: Fix some issues with positioning of initially-visible items The old implementation had a few problems: - For a target area near but not touching the bottom of the parent area, and a long menu with an initially-visible item halfway through the list, the initially-visible item would sometimes be obscured by the scroller area. The new implementation adjusts the size of the menu to ensure that there's enough additional room for the scroller areas. - For a target area at the very top or very bottom of the target area, the menu would be positioned inconsistently; sometimes it would overlap with the target area, but other times it would be positioned with a gap separating the menu and target area. In the new implementation, if there's not enough room for scrollers to be positioned above/below the target area, the menu will always be positioned so that it touches but does not overlap the target area. - The initially-selected item would normally be positioned as close as possible to the target area, but this wasn't always applied consistently for long menus, and the menu would sometimes only be scrolled enough to make the item visible at the end of the menu furthest from the target area. In the new implementation, the initially-visible item will always be positioned on top of the target area, or adjacent to it, depending on the available space. --- .../juce_gui_basics/menus/juce_PopupMenu.cpp | 72 ++++++++++--------- 1 file changed, 40 insertions(+), 32 deletions(-) diff --git a/modules/juce_gui_basics/menus/juce_PopupMenu.cpp b/modules/juce_gui_basics/menus/juce_PopupMenu.cpp index f8706d4001..12efb29c3a 100644 --- a/modules/juce_gui_basics/menus/juce_PopupMenu.cpp +++ b/modules/juce_gui_basics/menus/juce_PopupMenu.cpp @@ -439,8 +439,7 @@ struct MenuWindow final : public Component return targetArea.getTopLeft(); }); - const auto y = targetPosition.getY() - windowPos.getY(); - ensureItemComponentIsVisible (**iter, isPositiveAndBelow (y, windowPos.getHeight()) ? std::optional (y) : std::nullopt); + ensureItemComponentIsVisible (**iter, targetPosition.getY() - windowPos.getY()); } } @@ -1095,7 +1094,7 @@ struct MenuWindow final : public Component { const auto parentArea = getParentArea (windowPos.getPosition(), options.getParentComponent()) / scaleFactor; - if (const auto posAndOffset = computeInitialPosAndOffset (windowPos, parentArea, itemComp.getBounds(), wantedY)) + if (const auto posAndOffset = computePosAndOffsetToEnsureVisibility (windowPos, parentArea, itemComp.getBounds(), contentHeight, wantedY)) { std::tie (windowPos, childYOffset) = std::tie (posAndOffset->windowPos, posAndOffset->childYOffset); updateYPositions(); @@ -1108,40 +1107,49 @@ struct MenuWindow final : public Component int childYOffset = 0; }; - static std::optional computeInitialPosAndOffset (Rectangle windowPos, - const Rectangle& parentArea, - const Rectangle& itemCompBounds, - std::optional wantedY) + static std::optional computePosAndOffsetToEnsureVisibility (Rectangle windowPos, + const Rectangle& parentArea, + const Rectangle& itemCompBounds, + int contentHeight, + std::optional wantedY) { - if (windowPos.getHeight() <= PopupMenuSettings::scrollZone * 4 - || (! wantedY.has_value() && 0 <= itemCompBounds.getY() && itemCompBounds.getBottom() <= windowPos.getHeight())) - { + // If there's no specific wantedY, and the item component is already visible, then we don't + // need to make any adjustments. + if (! wantedY.has_value() && 0 <= itemCompBounds.getY() && itemCompBounds.getBottom() <= windowPos.getHeight()) return {}; + + const auto spaceNeededAboveItem = jmin (PopupMenuSettings::scrollZone, itemCompBounds.getY()); + const auto spaceNeededBelowItem = jmin (PopupMenuSettings::scrollZone, contentHeight - itemCompBounds.getBottom()); + const auto parentSpaceTargetY = windowPos.getY() + wantedY.value_or (itemCompBounds.getY()); + + // In order to display the visible item over the target area, we need to make sure that + // there's enough space above and below to hold the scroll areas if they're showing. + // Ideally, we want to avoid the case where the menu opens with the scroll area over the + // target area. + const auto isSpaceToOverlay = spaceNeededAboveItem <= (parentSpaceTargetY - parentArea.getY()) + && spaceNeededBelowItem <= (parentArea.getBottom() - (parentSpaceTargetY + itemCompBounds.getHeight())); + + if (wantedY.has_value() && isSpaceToOverlay) + { + windowPos = windowPos.withY (parentSpaceTargetY - itemCompBounds.getY()) + .withHeight (contentHeight) + .constrainedWithin (parentArea); + + const auto menuSpaceTargetY = parentSpaceTargetY - windowPos.getY(); + const auto offset = itemCompBounds.getY() - menuSpaceTargetY; + + return PosAndOffset { windowPos, offset }; } - const auto adjustedY = std::invoke ([&] - { - if (wantedY.has_value()) - return *wantedY; + // If there's not enough space to overlay the menu, then just use the provided menu + // bounds but try to position the visible item as close to the target area as possible, + // while avoiding the scroll areas. + const auto menuSpaceTargetY = jlimit (spaceNeededAboveItem, + windowPos.getHeight() - spaceNeededBelowItem - itemCompBounds.getHeight(), + parentSpaceTargetY - windowPos.getY()); + const auto offset = itemCompBounds.getY() - menuSpaceTargetY; - return jlimit (PopupMenuSettings::scrollZone, - jmax (PopupMenuSettings::scrollZone, - windowPos.getHeight() - (PopupMenuSettings::scrollZone + itemCompBounds.getHeight())), - itemCompBounds.getY()); - }); - - const auto deltaY = windowPos.getY() + adjustedY - itemCompBounds.getY(); - - windowPos.setSize (jmin (windowPos.getWidth(), parentArea.getWidth()), - jmin (windowPos.getHeight(), parentArea.getHeight())); - - const auto newY = jlimit (parentArea.getY(), - parentArea.getBottom() - windowPos.getHeight(), - deltaY); - - windowPos.setY (newY); - - return PosAndOffset { windowPos, newY - deltaY }; + return PosAndOffset { windowPos, offset }; } void resizeToBestWindowPos()