Fix cells highlighting in different modes

duckduckgo · Aug 30, 2024 · db4b4ab · db4b4ab
1 parent bc2fec3
commit db4b4ab
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 35 deletions.
diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj
@@ -6056,6 +6056,8 @@
 				4BE5336A286912D40019DBFD /* BookmarksBarCollectionViewItem.swift */,
 				4BE53369286912D40019DBFD /* BookmarksBarCollectionViewItem.xib */,
 				85774AFE2A713D3B00DE0561 /* BookmarksBarMenuFactory.swift */,
+				84F1C8CE2C7705B500716446 /* BookmarksBarMenuPopover.swift */,
+				848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */,
 				4BD18EFF283F0BC500058124 /* BookmarksBarViewController.swift */,
 				4BE41A5D28446EAD00760399 /* BookmarksBarViewModel.swift */,
 				4BE5336D286915A10019DBFD /* HorizontallyCenteredLayout.swift */,
@@ -6482,8 +6484,8 @@
 		859F30622A72A7A900C20372 /* Prompt */ = {
 			isa = PBXGroup;
 			children = (
-				859F30632A72A7BB00C20372 /* BookmarksBarPromptPopover.swift */,
 				859F30662A72B38500C20372 /* BookmarksBarPromptAssets.xcassets */,
+				859F30632A72A7BB00C20372 /* BookmarksBarPromptPopover.swift */,
 			);
 			path = Prompt;
 			sourceTree = "<group>";
@@ -7673,8 +7675,6 @@
 				4B9292C72667123700AD2C21 /* BookmarkManagementSidebarViewController.swift */,
 				4B9292C82667123700AD2C21 /* BookmarkManagementSplitViewController.swift */,
 				4B92928726670D1600AD2C21 /* BookmarkOutlineCellView.swift */,
-				84F1C8CE2C7705B500716446 /* BookmarksBarMenuPopover.swift */,
-				848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */,
 				4B92928526670D1600AD2C21 /* BookmarksOutlineView.swift */,
 				4B92928926670D1700AD2C21 /* BookmarkTableCellView.swift */,
 				4B9292C92667123700AD2C21 /* BookmarkTableRowView.swift */,

diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift b/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift
@@ -22,7 +22,7 @@ import Foundation
 
 final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSource, NSOutlineViewDelegate {
 
-    enum ContentMode {
+    enum ContentMode: CaseIterable {
         case bookmarksAndFolders
         case foldersOnly
         case bookmarksMenu
@@ -33,6 +33,13 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou
             case .foldersOnly: false
             }
         }
+
+        var showMenuButtonOnHover: Bool {
+            switch self {
+            case .bookmarksAndFolders, .bookmarksMenu: true
+            case .foldersOnly: false
+            }
+        }
     }
 
     @Published var selectedFolders: [BookmarkFolder] = []
@@ -72,7 +79,6 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou
     private let treeController: BookmarkTreeController
     private let bookmarkManager: BookmarkManager
     private let dragDropManager: BookmarkDragDropManager
-    private let showMenuButtonOnHover: Bool
     private let presentFaviconsFetcherOnboarding: (() -> Void)?
 
     init(
@@ -81,14 +87,12 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou
         treeController: BookmarkTreeController,
         dragDropManager: BookmarkDragDropManager = .shared,
         sortMode: BookmarksSortMode,
-        showMenuButtonOnHover: Bool = true,
         presentFaviconsFetcherOnboarding: (() -> Void)? = nil
     ) {
         self.contentMode = contentMode
         self.bookmarkManager = bookmarkManager
         self.dragDropManager = dragDropManager
         self.treeController = treeController
-        self.showMenuButtonOnHover = showMenuButtonOnHover
         self.presentFaviconsFetcherOnboarding = presentFaviconsFetcherOnboarding
 
         super.init()
@@ -205,11 +209,10 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou
                 ?? OutlineSeparatorViewCell(isSeparatorVisible: contentMode.isSeparatorVisible)
         }
 
-        let cell = outlineView.makeView(withIdentifier: .init(BookmarkOutlineCellView.className()), owner: self) as? BookmarkOutlineCellView
-            ?? BookmarkOutlineCellView(identifier: .init(BookmarkOutlineCellView.className()))
-        cell.shouldShowMenuButton = showMenuButtonOnHover
+        let cell = outlineView.makeView(withIdentifier: BookmarkOutlineCellView.identifier(for: contentMode), owner: self) as? BookmarkOutlineCellView
+            ?? BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: contentMode))
         cell.delegate = self
-        cell.update(from: node, isSearch: isSearching, isMenuPopover: contentMode == .bookmarksMenu)
+        cell.update(from: node, isSearch: isSearching)
 
         if let bookmark = node.representedObject as? Bookmark, bookmark.favicon(.small) == nil {
             presentFaviconsFetcherOnboarding?()

diff --git a/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift b/DuckDuckGo/Bookmarks/View/BookmarkManagementSidebarViewController.swift
@@ -56,8 +56,7 @@ final class BookmarkManagementSidebarViewController: NSViewController {
                                                                 bookmarkManager: bookmarkManager,
                                                                 treeController: treeController,
                                                                 dragDropManager: dragDropManager,
-                                                                sortMode: selectedSortMode,
-                                                                showMenuButtonOnHover: false)
+                                                                sortMode: selectedSortMode)
 
     private var cancellables = Set<AnyCancellable>()
     private var selectedSortMode: BookmarksSortMode

diff --git a/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift b/DuckDuckGo/Bookmarks/View/BookmarkOutlineCellView.swift
@@ -26,6 +26,10 @@ protocol BookmarkOutlineCellViewDelegate: AnyObject {
 final class BookmarkOutlineCellView: NSTableCellView {
 
     private static let sizingCellIdentifier = NSUserInterfaceItemIdentifier("sizing")
+    static func identifier(for mode: BookmarkOutlineViewDataSource.ContentMode) -> NSUserInterfaceItemIdentifier {
+        NSUserInterfaceItemIdentifier("\(mode)_\(self.className())")
+    }
+
     static let sizingCell = BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.sizingCellIdentifier)
 
     static let rowHeight: CGFloat = 28
@@ -51,7 +55,18 @@ final class BookmarkOutlineCellView: NSTableCellView {
         }
     }
 
-    var shouldShowMenuButton = false
+    var contentMode: BookmarkOutlineViewDataSource.ContentMode? {
+        BookmarkOutlineViewDataSource.ContentMode.allCases.first { mode in
+            Self.identifier(for: mode) == self.identifier
+        }
+    }
+
+    var shouldShowMenuButton: Bool {
+        contentMode != .foldersOnly
+    }
+    var shouldShowChevron: Bool {
+        contentMode == .bookmarksMenu
+    }
 
     weak var delegate: BookmarkOutlineCellViewDelegate?
 
@@ -222,7 +237,9 @@ final class BookmarkOutlineCellView: NSTableCellView {
         }
         if !titleLabel.isEnabled {
             titleLabel.textColor = .disabledControlTextColor
-        } else if highlight && isInKeyWindow {
+        } else if highlight,
+                  isInKeyWindow,
+                  contentMode != .foldersOnly {
             titleLabel.textColor = .selectedMenuItemTextColor
             urlLabel.textColor = .selectedMenuItemTextColor
         } else {
@@ -253,19 +270,19 @@ final class BookmarkOutlineCellView: NSTableCellView {
                 || representedObject is PseudoFolder || representedObject is MenuItemNode else { return 0 }
 
         sizingCell.frame = .zero
-        sizingCell.update(from: representedObject, isMenuPopover: true)
+        sizingCell.update(from: representedObject)
         sizingCell.layoutSubtreeIfNeeded()
 
         return sizingCell.frame.width + 6
     }
 
-    func update(from object: Any, isSearch: Bool = false, isMenuPopover: Bool) {
+    func update(from object: Any, isSearch: Bool = false) {
         let representedObject = (object as? BookmarkNode)?.representedObject ?? object
         switch representedObject {
         case let bookmark as Bookmark:
             update(from: bookmark, isSearch: isSearch, showURL: identifier != Self.sizingCellIdentifier)
         case let folder as BookmarkFolder:
-            update(from: folder, isSearch: isSearch, showChevron: isMenuPopover)
+            update(from: folder, isSearch: isSearch)
         case let folder as PseudoFolder:
             update(from: folder)
         case let menuItem as MenuItemNode:
@@ -298,18 +315,18 @@ final class BookmarkOutlineCellView: NSTableCellView {
         updateConstraints(isSearch: isSearch)
     }
 
-    func update(from folder: BookmarkFolder, isSearch: Bool = false, showChevron: Bool) {
+    func update(from folder: BookmarkFolder, isSearch: Bool = false) {
         faviconImageView.image = .folder
         faviconImageView.isHidden = false
         titleLabel.stringValue = folder.title
         titleLabel.isEnabled = true
-        favoriteImageView.image = showChevron ? .chevronMediumRight16 : nil
+        favoriteImageView.image = shouldShowChevron ? .chevronMediumRight16 : nil
         favoriteImageView.isHidden = favoriteImageView.image == nil
         urlLabel.stringValue = ""
         self.toolTip = nil
 
         let totalChildBookmarks = folder.totalChildBookmarks
-        if totalChildBookmarks > 0 && !showChevron {
+        if totalChildBookmarks > 0 && !shouldShowChevron {
             countLabel.stringValue = String(totalChildBookmarks)
             countLabel.isHidden = false
         } else {
@@ -373,12 +390,12 @@ extension BookmarkOutlineCellView {
                 BookmarkOutlineCellView(identifier: .init("")),
                 BookmarkOutlineCellView(identifier: .init("")),
                 BookmarkOutlineCellView(identifier: .init("")),
+                BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)),
                 BookmarkOutlineCellView(identifier: .init("")),
                 BookmarkOutlineCellView(identifier: .init("")),
                 BookmarkOutlineCellView(identifier: .init("")),
-                BookmarkOutlineCellView(identifier: .init("")),
-                BookmarkOutlineCellView(identifier: .init("")),
-                BookmarkOutlineCellView(identifier: .init("")),
+                BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)),
+                BookmarkOutlineCellView(identifier: BookmarkOutlineCellView.identifier(for: .bookmarksMenu)),
                 BookmarkOutlineCellView(identifier: .init("")),
                 BookmarkOutlineCellView(identifier: .init("")),
             ]
@@ -397,22 +414,22 @@ extension BookmarkOutlineCellView {
             let bkm2 = Bookmark(id: "3", url: "http://a.b", title: "Bookmark with longer title to test width", isFavorite: false)
             cells[2].update(from: bkm2, showURL: false)
 
-            cells[3].update(from: BookmarkFolder(id: "4", title: "Bookmark Folder with a reasonably long name"), showChevron: true)
-            cells[4].update(from: BookmarkFolder(id: "5", title: "Bookmark Folder with 42 bookmark children", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), showChevron: false)
+            cells[3].update(from: BookmarkFolder(id: "4", title: "Bookmark Folder with a reasonably long name"))
+            cells[4].update(from: BookmarkFolder(id: "5", title: "Bookmark Folder with 42 bookmark children", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)))
             PseudoFolder.favorites.count = 64
             cells[5].update(from: PseudoFolder.favorites)
             PseudoFolder.bookmarks.count = 256
             cells[6].update(from: PseudoFolder.bookmarks)
 
             let node = BookmarkNode(representedObject: MenuItemNode(identifier: "", title: UserText.bookmarksOpenInNewTabs, isEnabled: true), parent: BookmarkNode.genericRootNode())
-            cells[7].update(from: node, isMenuPopover: true)
+            cells[7].update(from: node)
 
             let emptyNode = BookmarkNode(representedObject: MenuItemNode(identifier: "", title: UserText.bookmarksBarFolderEmpty, isEnabled: false), parent: BookmarkNode.genericRootNode())
-            cells[8].update(from: emptyNode, isMenuPopover: true)
+            cells[8].update(from: emptyNode)
 
             let sbkm = Bookmark(id: "3", url: "http://a.b", title: "Bookmark in Search mode", isFavorite: false)
             cells[9].update(from: sbkm, isSearch: true, showURL: false)
-            cells[10].update(from: BookmarkFolder(id: "5", title: "Folder in Search mode", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), isSearch: true, showChevron: false)
+            cells[10].update(from: BookmarkFolder(id: "5", title: "Folder in Search mode", children: Array(repeating: Bookmark(id: "2", url: "http://a.b", title: "DuckDuckGo", isFavorite: true), count: 42)), isSearch: true)
 
             widthAnchor.constraint(equalToConstant: 258).isActive = true
             heightAnchor.constraint(equalToConstant: CGFloat((28 + 1) * cells.count)).isActive = true

diff --git a/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift b/DuckDuckGo/Bookmarks/View/BookmarksOutlineView.swift
@@ -83,6 +83,9 @@ final class BookmarksOutlineView: NSOutlineView {
         return nil
     }
 
+    private var isInPopover: Bool {
+        popover != nil
+    }
     private var isInKeyPopover: Bool {
         guard highlightedRow != nil else { return false }
         // is there a child menu popover window owned by our window?
@@ -103,6 +106,11 @@ final class BookmarksOutlineView: NSOutlineView {
 
     // mark highlight with inactive color for non-key popover menu and with active color for key popover menu
     private func updateIsInKeyPopoverState() {
+        guard isInPopover else {
+            highlightedRowView?.isInKeyWindow = false
+            highlightedCellView?.isInKeyWindow = false
+            return
+        }
         // when no highlighted row - our parent is the key popover
         guard highlightedRow != nil else {
             parentMenuOutlineView?.updateIsInKeyPopoverState()
@@ -366,7 +374,7 @@ final class BookmarksOutlineView: NSOutlineView {
         }
         if highlightedRow != row {
             highlightedRow = row
-        } else {
+        } else if isInPopover {
             highlightedRowView?.isInKeyWindow = true
             highlightedCellView?.isInKeyWindow = true
         }

diff --git a/...kmarks/View/BookmarksBarMenuPopover.swift → ...rksBar/View/BookmarksBarMenuPopover.swift b/...kmarks/View/BookmarksBarMenuPopover.swift → ...rksBar/View/BookmarksBarMenuPopover.swift
diff --git a/...View/BookmarksBarMenuViewController.swift → ...View/BookmarksBarMenuViewController.swift b/...View/BookmarksBarMenuViewController.swift → ...View/BookmarksBarMenuViewController.swift
diff --git a/Submodules/privacy-reference-tests b/Submodules/privacy-reference-tests
diff --git a/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift b/UnitTests/Bookmarks/Model/BookmarkOutlineViewDataSourceTests.swift
@@ -223,35 +223,54 @@ class BookmarkOutlineViewDataSourceTests: XCTestCase {
     }
 
     @MainActor
-    func testWhenShowMenuButtonOnHoverIsTrue_ThenCellShouldHaveShouldMenuButtonFlagTrue() throws {
+    func testWhenContentModeIsBookmarksAndFolders_ThenCellShouldHaveShouldMenuButtonFlagTrue() throws {
         // GIVEN
         let mockFolder = BookmarkFolder.mock
         let mockOutlineView = NSOutlineView(frame: .zero)
         let treeController = createTreeController(with: [mockFolder])
         let mockFolderNode = treeController.node(representing: mockFolder)!
-        let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksAndFolders, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual, showMenuButtonOnHover: true)
+        let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksAndFolders, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual)
 
         // WHEN
         let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView)
 
         // THEN
         XCTAssertTrue(cell.shouldShowMenuButton)
+        XCTAssertFalse(cell.shouldShowChevron)
     }
 
     @MainActor
-    func testWhenShowMenuButtonOnHoverIsFalse_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws {
+    func testWhenContentModeIsFolders_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws {
         // GIVEN
         let mockFolder = BookmarkFolder.mock
         let mockOutlineView = NSOutlineView(frame: .zero)
         let treeController = createTreeController(with: [mockFolder])
         let mockFolderNode = treeController.node(representing: mockFolder)!
-        let dataSource = BookmarkOutlineViewDataSource(contentMode: .foldersOnly, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual, showMenuButtonOnHover: false)
+        let dataSource = BookmarkOutlineViewDataSource(contentMode: .foldersOnly, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual)
 
         // WHEN
         let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView)
 
         // THEN
         XCTAssertFalse(cell.shouldShowMenuButton)
+        XCTAssertFalse(cell.shouldShowChevron)
+    }
+
+    @MainActor
+    func testWhenContentModeIsBookmarksMenu_ThenCellShouldHaveShouldMenuButtonFlagFalse() throws {
+        // GIVEN
+        let mockFolder = BookmarkFolder.mock
+        let mockOutlineView = NSOutlineView(frame: .zero)
+        let treeController = createTreeController(with: [mockFolder])
+        let mockFolderNode = treeController.node(representing: mockFolder)!
+        let dataSource = BookmarkOutlineViewDataSource(contentMode: .bookmarksMenu, bookmarkManager: LocalBookmarkManager(), treeController: treeController, sortMode: .manual)
+
+        // WHEN
+        let cell = try XCTUnwrap(dataSource.outlineView(mockOutlineView, viewFor: nil, item: mockFolderNode) as? BookmarkOutlineCellView)
+
+        // THEN
+        XCTAssertTrue(cell.shouldShowMenuButton)
+        XCTAssertTrue(cell.shouldShowChevron)
     }
 
     // MARK: - Private
+16 −18		.github/workflows/test.yml
+2 −3		fingerprinting-protections/README.md
+4 −6		fingerprinting-protections/init.js