From 54e6d0c797045c0867d18fa3b5afe2ad9c58d763 Mon Sep 17 00:00:00 2001 From: JimmFly Date: Tue, 26 May 2026 07:29:03 +0000 Subject: [PATCH] Fix history popup row hover spacing --- docs/plans/history-list-spacing.md | 34 ++++++++ ...ConversationListLiquidVirtualization.swift | 22 +++-- .../Conversation/ConversationListMenu.swift | 43 ++++++---- .../ConversationListMenuRowViews.swift | 1 + ...rsationListLiquidVirtualizationTests.swift | 83 ++++++++++++++++++- 5 files changed, 157 insertions(+), 26 deletions(-) create mode 100644 docs/plans/history-list-spacing.md diff --git a/docs/plans/history-list-spacing.md b/docs/plans/history-list-spacing.md new file mode 100644 index 0000000..b7e8d1f --- /dev/null +++ b/docs/plans/history-list-spacing.md @@ -0,0 +1,34 @@ +# History List Spacing Plan + +## Goal +Fix the small-window Bridge history popup so adjacent conversation rows no longer visually collide when a row is hovered. + +## Current State +- The macOS 26 small-window header uses `ConversationHistoryLiquidActionGroup` with the `.liquidPopup` conversation list style. +- Liquid popup conversation rows are spaced tightly, and non-virtualized rows are not forced to the same layout height as their hover/active background. +- In dense lists, an active row background and the next hovered row background can visually collide because the background extends outside the row's allocated geometry. + +## Proposed Changes +1. Keep the data flow, selection, rename/delete/share actions, shortcuts, and accessibility identifiers unchanged. +2. Restore the previous compact `.liquidPopup` row height while keeping hover backgrounds contained: 36px rows, 0px adjacent row gaps, and 12px section gaps. +3. Keep each compact history item to one visible title line by hiding the gray last-message preview text. +4. Force non-virtualized `.liquidPopup` item rows to use the same fixed row height that the background and virtualization layout use. +5. Keep the legacy `.menu` style unchanged. +6. Add regression coverage for the liquid popup row metrics, preview visibility, inter-section spacing, and virtualization layout gaps. +7. Validate the popup in a small-window history scenario and capture a screenshot. + +## What Happens If We Do Not Change It +Hovering a conversation row in the small-window history popup can still overlap or crowd adjacent rows, making the list feel broken and increasing mis-click risk. + +## Expected Result After Change +Each history item keeps its hover/active background inside its own row box. Adjacent item backgrounds meet without an inserted gap and without visual overlap. + +## Acceptance Criteria +- `.liquidPopup` adjacent rows have a 0px layout gap while each hover/active background stays contained in its own row box. +- `.liquidPopup` rows do not render the gray last-message preview text. +- Legacy `.menu` row metrics remain unchanged. +- Virtualized and non-virtualized liquid list rows use the previous compact height while keeping hover/active backgrounds inside that row box. +- Section headers keep a clear gap from the previous section's final row background. +- Existing row actions and selection behavior remain unchanged. +- Automated coverage passes or any environment blocker is explicitly reported. +- Manual small-window visual validation produces a screenshot showing separated hover rows. diff --git a/macos/OpenBridge/Interface/Conversation/ConversationListLiquidVirtualization.swift b/macos/OpenBridge/Interface/Conversation/ConversationListLiquidVirtualization.swift index fbab91f..9647bea 100644 --- a/macos/OpenBridge/Interface/Conversation/ConversationListLiquidVirtualization.swift +++ b/macos/OpenBridge/Interface/Conversation/ConversationListLiquidVirtualization.swift @@ -87,16 +87,20 @@ enum ConversationListLiquidVirtualization { var cursorY = style.verticalPadding for (sectionIndex, section) in sections.enumerated() { - if sectionIndex > 0, style.showsSectionDivider { - rows.append( - Row( - id: "divider-\(sectionIndex)", - minY: cursorY, - height: style.dividerHeight, - kind: .divider + if sectionIndex > 0 { + if style.showsSectionDivider { + rows.append( + Row( + id: "divider-\(sectionIndex)", + minY: cursorY, + height: style.dividerHeight, + kind: .divider + ) ) - ) - cursorY += style.dividerHeight + cursorY += style.dividerHeight + } else { + cursorY += style.sectionSpacing + } } rows.append( diff --git a/macos/OpenBridge/Interface/Conversation/ConversationListMenu.swift b/macos/OpenBridge/Interface/Conversation/ConversationListMenu.swift index 3c778cf..ef5db06 100644 --- a/macos/OpenBridge/Interface/Conversation/ConversationListMenu.swift +++ b/macos/OpenBridge/Interface/Conversation/ConversationListMenu.swift @@ -1063,7 +1063,21 @@ enum ConversationListPresentationStyle: Equatable { } var rowSpacing: CGFloat { - 2 + switch self { + case .menu: + 2 + case .liquidPopup: + 0 + } + } + + var sectionSpacing: CGFloat { + switch self { + case .menu: + 0 + case .liquidPopup: + 12 + } } var rowHorizontalPadding: CGFloat { @@ -1094,12 +1108,7 @@ enum ConversationListPresentationStyle: Equatable { } var rowHoverBackgroundHeight: CGFloat { - switch self { - case .menu: - rowHeight - case .liquidPopup: - 38 - } + rowHeight } var rowOuterHorizontalPadding: CGFloat { @@ -1121,12 +1130,7 @@ enum ConversationListPresentationStyle: Equatable { } var showsPreview: Bool { - switch self { - case .menu: - false - case .liquidPopup: - true - } + false } var sectionFont: Font { @@ -1323,8 +1327,12 @@ struct ConversationListMenuContent: View { private func listContent(_ groups: [ConversationListSection]) -> some View { VStack(alignment: .leading, spacing: 0) { ForEach(Array(groups.enumerated()), id: \.offset) { index, section in - if index > 0, style.showsSectionDivider { - Divider().padding(.vertical, 4) + if index > 0 { + if style.showsSectionDivider { + Divider().padding(.vertical, 4) + } else if style.sectionSpacing > 0 { + Color.clear.frame(height: style.sectionSpacing) + } } sectionView(section) } @@ -1373,6 +1381,7 @@ struct ConversationListMenuContent: View { ForEach(section.items, id: \.id) { session in sessionRow(session) .padding(.horizontal, style.rowOuterHorizontalPadding) + .frame(height: style.rowHeight) } } } @@ -1383,7 +1392,9 @@ struct ConversationListMenuContent: View { var height: CGFloat = padding for (index, section) in groups.enumerated() { - if index > 0, style.showsSectionDivider { height += style.dividerHeight } + if index > 0 { + height += style.showsSectionDivider ? style.dividerHeight : style.sectionSpacing + } height += style.sectionHeaderHeight height += CGFloat(section.items.count) * style.rowHeight height += CGFloat(max(section.items.count - 1, 0)) * style.rowSpacing diff --git a/macos/OpenBridge/Interface/Conversation/ConversationListMenuRowViews.swift b/macos/OpenBridge/Interface/Conversation/ConversationListMenuRowViews.swift index c85a292..e7e3d0e 100644 --- a/macos/OpenBridge/Interface/Conversation/ConversationListMenuRowViews.swift +++ b/macos/OpenBridge/Interface/Conversation/ConversationListMenuRowViews.swift @@ -54,6 +54,7 @@ struct SessionRowButton: View { .padding(.vertical, style.rowVerticalPadding) } .frame(maxWidth: .infinity, alignment: .leading) + .frame(height: style.rowHeight) .background(alignment: .center) { rowBackground .frame(height: style.rowHoverBackgroundHeight) diff --git a/macos/OpenBridgeUnitTests/Suites/ConversationListLiquidVirtualizationTests.swift b/macos/OpenBridgeUnitTests/Suites/ConversationListLiquidVirtualizationTests.swift index f693f17..55175e7 100644 --- a/macos/OpenBridgeUnitTests/Suites/ConversationListLiquidVirtualizationTests.swift +++ b/macos/OpenBridgeUnitTests/Suites/ConversationListLiquidVirtualizationTests.swift @@ -1,3 +1,4 @@ +import CoreGraphics @testable import OpenBridge import Testing @@ -47,7 +48,87 @@ struct ConversationListLiquidVirtualizationTests { #expect(visibleRows.count < layout.rows.count) #expect(visibleRows.first?.id == "session-3") - #expect(visibleRows.last?.id == "session-5") + #expect(visibleRows.last?.id == "session-6") + } + + @Test + func `menu row metrics stay compact for legacy history menu`() { + let style = ConversationListPresentationStyle.menu + + #expect(style.rowHeight == 30) + #expect(style.rowSpacing == 2) + #expect(style.sectionSpacing == 0) + #expect(style.rowHoverBackgroundHeight == style.rowHeight) + } + + @Test + func `liquid popup row metrics do not add a gap between adjacent row backgrounds`() { + let style = ConversationListPresentationStyle.liquidPopup + let hoverOverhang = max(CGFloat.zero, style.rowHoverBackgroundHeight - style.rowHeight) + let overlapBetweenAdjacentHoverBackgrounds = hoverOverhang - style.rowSpacing + + #expect(style.rowHeight == 36) + #expect(style.rowVerticalPadding == 3) + #expect(!style.showsPreview) + #expect(style.rowHoverBackgroundHeight <= style.rowHeight) + #expect(style.rowSpacing == 0) + #expect(overlapBetweenAdjacentHoverBackgrounds <= 0) + #expect(style.sectionSpacing >= 12) + } + + @Test + func `liquid popup layout keeps consecutive session rows hover safe`() { + let layout = ConversationListLiquidVirtualization.buildLayout( + sections: [ + ConversationListSection( + title: "today", + items: [ + session(id: "a", updatedAt: 300), + session(id: "b", updatedAt: 200), + ] + ), + ], + style: .liquidPopup, + includesLoadMoreFooter: false, + loadMoreFooterHeight: 28 + ) + let sessionRows = layout.rows.filter { row in + if case .session = row.kind { + return true + } + return false + } + + #expect(sessionRows.count == 2) + let gap = sessionRows[1].minY - sessionRows[0].maxY + #expect(gap == 0) + } + + @Test + func `liquid popup layout keeps section headers clear of preceding row hover`() throws { + let layout = ConversationListLiquidVirtualization.buildLayout( + sections: [ + ConversationListSection( + title: "yesterday", + items: [ + session(id: "recent", updatedAt: 300), + ] + ), + ConversationListSection( + title: "3w ago", + items: [ + session(id: "older", updatedAt: 200), + ] + ), + ], + style: .liquidPopup, + includesLoadMoreFooter: false, + loadMoreFooterHeight: 28 + ) + let recentRow = try #require(layout.rows.first { $0.id == "recent" }) + let olderHeader = try #require(layout.rows.first { $0.id == "header-1-3w ago" }) + + #expect(olderHeader.minY - recentRow.maxY >= ConversationListPresentationStyle.liquidPopup.sectionSpacing) } @Test