diff --git a/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift b/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift index 88fc75ddbdd6..919f73f65732 100644 --- a/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift +++ b/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/ArrowlessPopoverAnchor.swift @@ -186,7 +186,10 @@ public struct ArrowlessPopoverAnchor: NSViewRepresentable of: anchorView, preferredEdge: preferredEdge ) - if popover.isShown { + // AppKit can report `isShown == false` while an opening animation is + // still in flight. Register immediately so animated roots participate + // in grouped dismissal just like immediately shown popovers. + if groupMemberID == nil { groupMemberID = group?.register(popover: popover, anchor: anchorView) } } diff --git a/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift b/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift index 556d948d4987..c8ccb763a2cf 100644 --- a/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift +++ b/Packages/macOS/CmuxAppKitSupportUI/Sources/CmuxAppKitSupportUI/Popover/CmuxPopoverGroup.swift @@ -80,6 +80,9 @@ public final class CmuxPopoverGroup { containsPointer: ((Int?, CGPoint) -> Bool)? = nil, close: @escaping () -> Void ) { + if parent == nil, !members.isEmpty { + dismissAll() + } members.append(Member( id: id, parent: parent, diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 2574ce3531fb..c19d7e001cbc 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -15425,7 +15425,7 @@ struct SidebarFooterButtons: View { private var workspacePresentationMode = WorkspacePresentationModeSettings.defaultMode.rawValue /// Owns the discovery popover so it persists after ⌘ is released. @State private var isShortcutPopoverPresented = false - + @State private var footerPopoverGroup = CmuxPopoverGroup() private var presentationMode: WorkspacePresentationModeSettings.Mode { WorkspacePresentationModeSettings.mode(for: workspacePresentationMode) } @@ -15439,13 +15439,13 @@ struct SidebarFooterButtons: View { if shows(.account) || shows(.mobileConnect) || shows(.help) { HStack(spacing: 0) { if shows(.account), CmuxFeatureFlags.shared.isSidebarAccountButtonEnabled { - SidebarAccountMenuButton() + SidebarAccountMenuButton(popoverGroup: footerPopoverGroup) } if shows(.mobileConnect), CmuxFeatureFlags.shared.isMobileConnectButtonEnabled { SidebarMobileConnectButton() } if shows(.help) { - SidebarHelpMenuButton(onSendFeedback: onSendFeedback) + SidebarHelpMenuButton(onSendFeedback: onSendFeedback, popoverGroup: footerPopoverGroup) } } } @@ -15517,7 +15517,7 @@ private struct SidebarHelpMenuButton: View { @State private var keyboardShortcutSettingsObserver = KeyboardShortcutSettingsObserver.shared let onSendFeedback: () -> Void - + let popoverGroup: CmuxPopoverGroup @State private var isPopoverPresented = false private var iconSize: CGFloat { @@ -15549,7 +15549,7 @@ private struct SidebarHelpMenuButton: View { var body: some View { Button { - isPopoverPresented.toggle() + let shouldPresent = !isPopoverPresented; popoverGroup.dismissAll(); isPopoverPresented = shouldPresent } label: { SidebarFooterHelpIcon(pointSize: iconSize, weight: iconWeight) .frame(width: buttonSize, height: buttonSize, alignment: .center) @@ -15559,7 +15559,7 @@ private struct SidebarHelpMenuButton: View { .background(ArrowlessPopoverAnchor( isPresented: $isPopoverPresented, preferredEdge: .maxY, - detachedGap: 4 + detachedGap: 4, presentationAnimation: .enabled, group: popoverGroup ) { helpPopover }) diff --git a/Sources/SidebarAccountTeamPopover.swift b/Sources/SidebarAccountTeamPopover.swift index aceb28921a7f..00697038ab9b 100644 --- a/Sources/SidebarAccountTeamPopover.swift +++ b/Sources/SidebarAccountTeamPopover.swift @@ -17,7 +17,7 @@ struct SidebarAccountMenuButton: View { private let buttonSize = SidebarFooterButtonMetrics.buttonSize @State private var isPopoverPresented = false @State private var isShowingTeamPicker = false - @State private var popoverGroup = CmuxPopoverGroup() + let popoverGroup: CmuxPopoverGroup #if DEBUG @AppStorage(SidebarFooterProfileIconDebugSettings.sizeKey) private var debugIconSize = SidebarFooterProfileIconDebugSettings.defaultSize @@ -71,7 +71,7 @@ struct SidebarAccountMenuButton: View { ) Button { if isSignedIn { - isPopoverPresented.toggle() + let shouldPresent = !isPopoverPresented; popoverGroup.dismissAll(); isPopoverPresented = shouldPresent } else { _ = AppDelegate.shared?.performAccountSignInWorkspaceAction( tabManager: tabManager, @@ -111,7 +111,7 @@ struct SidebarAccountMenuButton: View { .task { for await _ in NotificationCenter.default.notifications(named: .cmuxTeamPickerShortcutRequested) { guard !Task.isCancelled else { return } - isPopoverPresented = true + popoverGroup.dismissAll(); isPopoverPresented = true } } .onChange(of: isPopoverPresented) { _, presented in diff --git a/cmuxTests/CmuxPopoverGroupTests.swift b/cmuxTests/CmuxPopoverGroupTests.swift index 957a7cbb4aa8..c2cee427b39f 100644 --- a/cmuxTests/CmuxPopoverGroupTests.swift +++ b/cmuxTests/CmuxPopoverGroupTests.swift @@ -1,10 +1,49 @@ import AppKit +import SwiftUI import Testing @testable import CmuxAppKitSupportUI @MainActor @Suite struct CmuxPopoverGroupTests { + @Test func animatedRootRegistersBeforeItsOpeningTransitionFinishes() { + let group = CmuxPopoverGroup() + let window = NSWindow( + contentRect: CGRect(x: 0, y: 0, width: 360, height: 240), + styleMask: [.borderless], + backing: .buffered, + defer: false + ) + let accountAnchor = NSView(frame: CGRect(x: 24, y: 160, width: 40, height: 24)) + let helpAnchor = NSView(frame: CGRect(x: 84, y: 160, width: 40, height: 24)) + window.contentView?.addSubview(accountAnchor) + window.contentView?.addSubview(helpAnchor) + window.orderFrontRegardless() + defer { window.close() } + + var accountPresented = true + let accountCoordinator = ArrowlessPopoverAnchor.Coordinator( + isPresented: Binding(get: { accountPresented }, set: { accountPresented = $0 }), + presentationAnimation: .automatic, + group: group + ) + accountCoordinator.anchorView = accountAnchor + accountCoordinator.updateRootView(AnyView(EmptyView())) + accountCoordinator.present(preferredEdge: .maxY, detachedGap: 4) + + let helpCoordinator = ArrowlessPopoverAnchor.Coordinator( + isPresented: .constant(true), + presentationAnimation: .enabled, + group: group + ) + helpCoordinator.anchorView = helpAnchor + helpCoordinator.updateRootView(AnyView(EmptyView())) + helpCoordinator.present(preferredEdge: .maxY, detachedGap: 4) + + #expect(!accountPresented) + group.dismissAll() + } + @Test func groupedPickerCanOptIntoNativeOpeningAnimation() { #expect( CmuxPopoverPresentationAnimation.enabled.animates( @@ -175,4 +214,78 @@ struct CmuxPopoverGroupTests { group.handleClick(windowNumber: nil, point: .zero) #expect(closed == [first, second]) } + + @Test func openingHelpClosesTheTeamPickerBeforeTheAccountMenu() { + let group = CmuxPopoverGroup() + let account = UUID() + let picker = UUID() + let help = UUID() + var closed: [UUID] = [] + group.register(id: account, parent: nil, contains: { _, _ in true }, close: { + closed.append(account) + group.unregister(account) + }) + group.register(id: picker, parent: account, contains: { _, _ in true }, close: { + closed.append(picker) + group.unregister(picker) + }) + #expect(closed.isEmpty) + + group.register(id: help, parent: nil, contains: { _, point in point.x < 220 }, close: { + closed.append(help) + group.unregister(help) + }) + #expect(closed == [picker, account]) + + group.handleClick(windowNumber: nil, point: CGPoint(x: 40, y: 80)) + #expect(closed == [picker, account]) + group.handleClick(windowNumber: nil, point: CGPoint(x: 600, y: 80)) + #expect(closed == [picker, account, help]) + } + + @Test(arguments: [false, true]) + func switchingBetweenFooterMenusLeavesOnlyTheLatestRoot(startWithAccount: Bool) { + let group = CmuxPopoverGroup() + let names = startWithAccount ? ["account", "help", "account"] : ["help", "account", "help"] + var closed: [String] = [] + var previousID: UUID? + + for (index, name) in names.enumerated() { + let id = UUID() + group.register(id: id, parent: nil, contains: { _, _ in true }, close: { + closed.append(name) + group.unregister(id) + }) + // Cleanup from the old root cannot retire the newly registered menu. + if let previousID { group.unregister(previousID) } + #expect(closed == Array(names.prefix(index))) + previousID = id + } + + group.dismissAll() + #expect(closed == names) + group.dismissAll() + #expect(closed == names) + } + + @Test func replacingAFooterRootDoesNotDismissAnotherWindowsGroup() { + let firstWindow = CmuxPopoverGroup() + let secondWindow = CmuxPopoverGroup() + var firstClosed = 0 + var secondClosed = 0 + firstWindow.register(id: UUID(), parent: nil, contains: { _, _ in true }, close: { + firstClosed += 1 + }) + secondWindow.register(id: UUID(), parent: nil, contains: { _, _ in true }, close: { + secondClosed += 1 + }) + + firstWindow.register(id: UUID(), parent: nil, contains: { _, _ in true }, close: {}) + #expect(firstClosed == 1) + #expect(secondClosed == 0) + firstWindow.dismissAll() + #expect(secondClosed == 0) + secondWindow.dismissAll() + #expect(secondClosed == 1) + } }