Fix bookmarks keyboard navigation (#3336)

Task/Issue URL: https://app.asana.com/0/1201048563534612/1208368435145582/f
duckduckgo · Oct 5, 2024 · 6a62ca0 · 6a62ca0
1 parent a599ccb
commit 6a62ca0
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 22 deletions.
diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift b/DuckDuckGo/Bookmarks/Model/BookmarkOutlineViewDataSource.swift
@@ -172,30 +172,34 @@ final class BookmarkOutlineViewDataSource: NSObject, BookmarksOutlineViewDataSou
         }
     }
 
-    func firstHighlightableRow(for _: BookmarksOutlineView) -> Int? {
-        nodeForItem(nil).childNodes.firstIndex { $0.canBeHighlighted }
+    func firstHighlightableRow(for outlineView: BookmarksOutlineView) -> Int? {
+        return (0..<outlineView.numberOfRows).first { row in
+            nodeForItem(outlineView.item(atRow: row)).canBeHighlighted
+        }
     }
 
     func nextHighlightableRow(inNextSection: Bool, for outlineView: BookmarksOutlineView, after row: Int) -> Int? {
         if inNextSection {
             return lastHighlightableRow(for: outlineView) // no sections support for now
         }
-        let nodes = nodeForItem(nil).childNodes
-        guard nodes.indices.contains(row + 1) else { return nil }
-        return nodes[(row + 1)...].firstIndex { $0.canBeHighlighted }
+        return ((row + 1)..<outlineView.numberOfRows).first { row in
+            nodeForItem(outlineView.item(atRow: row)).canBeHighlighted
+        }
     }
 
     func previousHighlightableRow(inPreviousSection: Bool, for outlineView: BookmarksOutlineView, before row: Int) -> Int? {
         if inPreviousSection {
             return firstHighlightableRow(for: outlineView) // no sections support for now
         }
-        let nodes = nodeForItem(nil).childNodes
-        guard nodes.indices.contains(row) else { return nil }
-        return nodes[..<row].lastIndex { $0.canBeHighlighted }
+        return (0..<row).last { row in
+            nodeForItem(outlineView.item(atRow: row)).canBeHighlighted
+        }
     }
 
-    func lastHighlightableRow(for _: BookmarksOutlineView) -> Int? {
-        nodeForItem(nil).childNodes.lastIndex { $0.canBeHighlighted }
+    func lastHighlightableRow(for outlineView: BookmarksOutlineView) -> Int? {
+        return (0..<outlineView.numberOfRows).last { row in
+            nodeForItem(outlineView.item(atRow: row)).canBeHighlighted
+        }
     }
 
     func outlineView(_ outlineView: NSOutlineView, viewFor tableColumn: NSTableColumn?, item: Any) -> NSView? {

diff --git a/DuckDuckGo/BookmarksBar/View/BookmarksBarCollectionViewItem.swift b/DuckDuckGo/BookmarksBar/View/BookmarksBarCollectionViewItem.swift
@@ -21,7 +21,7 @@ import Cocoa
 protocol BookmarksBarCollectionViewItemDelegate: AnyObject {
 
     @MainActor func bookmarksBarCollectionViewItemClicked(_ item: BookmarksBarCollectionViewItem)
-    @MainActor func bookmarksBarCollectionViewItem(_ item: BookmarksBarCollectionViewItem, isMouseOver: Bool)
+    @MainActor func bookmarksBarCollectionViewItemMouseDidHover(_ item: BookmarksBarCollectionViewItem)
 
     func showDialog(_ dialog: any ModalView)
 
@@ -158,7 +158,13 @@ extension BookmarksBarCollectionViewItem: BookmarksContextMenuDelegate {
 extension BookmarksBarCollectionViewItem: MouseOverViewDelegate {
 
     func mouseOverView(_ mouseOverView: MouseOverView, isMouseOver: Bool) {
-        delegate?.bookmarksBarCollectionViewItem(self, isMouseOver: isMouseOver)
+        if isMouseOver {
+            delegate?.bookmarksBarCollectionViewItemMouseDidHover(self)
+        }
+    }
+
+    override func mouseMoved(with event: NSEvent) {
+        delegate?.bookmarksBarCollectionViewItemMouseDidHover(self)
     }
 
 }
diff --git a/DuckDuckGo/BookmarksBar/View/BookmarksBarViewController.swift b/DuckDuckGo/BookmarksBar/View/BookmarksBarViewController.swift
@@ -327,19 +327,24 @@ extension BookmarksBarViewController: BookmarksBarViewModelDelegate {
     }
 
     func mouseDidHover(over sender: Any) {
-        guard let bookmarkMenuPopover, bookmarkMenuPopover.isShown else { return }
-        var bookmarkFolder: BookmarkFolder?
+        guard let bookmarkMenuPopover, bookmarkMenuPopover.isShown,
+              NSApp.currentEvent.map({ NSPoint(x: $0.deltaX, y: $0.deltaY) }) != .zero else { return }
+        var bookmarkFolder: (() -> BookmarkFolder?)?
+        var bookmarkFolderId: String?
         var view: NSView?
         if let item = sender as? BookmarksBarCollectionViewItem {
-            bookmarkFolder = item.representedObject as? BookmarkFolder
+            let folder = item.representedObject as? BookmarkFolder
+            bookmarkFolder = { folder }
+            bookmarkFolderId = folder?.id
             view = item.view
         } else if let button = sender as? NSButton, button === clippedItemsIndicator {
-            bookmarkFolder = clippedItemsBookmarkFolder()
+            bookmarkFolder = { self.clippedItemsBookmarkFolder() }
+            bookmarkFolderId = PseudoFolder.bookmarks.id
             view = button
         }
-        if let bookmarkFolder, let view {
+        if let bookmarkFolderId, let view {
             // already shown?
-            guard bookmarkMenuPopover.rootFolder?.id != bookmarkFolder.id else { return }
+            guard bookmarkMenuPopover.rootFolder?.id != bookmarkFolderId, let bookmarkFolder = bookmarkFolder?() else { return }
             showSubmenu(for: bookmarkFolder, from: view)
         } else {
             bookmarkMenuPopover.close()

diff --git a/DuckDuckGo/BookmarksBar/View/BookmarksBarViewModel.swift b/DuckDuckGo/BookmarksBar/View/BookmarksBarViewModel.swift
@@ -400,10 +400,8 @@ extension BookmarksBarViewModel: BookmarksBarCollectionViewItemDelegate {
         delegate?.showDialog(dialog)
     }
 
-    func bookmarksBarCollectionViewItem(_ item: BookmarksBarCollectionViewItem, isMouseOver: Bool) {
-        if isMouseOver {
-            delegate?.mouseDidHover(over: item)
-        }
+    func bookmarksBarCollectionViewItemMouseDidHover(_ item: BookmarksBarCollectionViewItem) {
+        delegate?.mouseDidHover(over: item)
     }
 
 }
diff --git a/DuckDuckGo/Common/View/AppKit/HoverTrackingArea.swift b/DuckDuckGo/Common/View/AppKit/HoverTrackingArea.swift
@@ -113,6 +113,7 @@ final class HoverTrackingArea: NSTrackingArea {
         if let view, !view.isMouseOver {
             view.isMouseOver = true
         }
+        view?.mouseMoved(with: event)
     }
 
     @objc func mouseExited(_ event: NSEvent) {

diff --git a/DuckDuckGo/Common/View/AppKit/MouseOverView.swift b/DuckDuckGo/Common/View/AppKit/MouseOverView.swift
@@ -22,6 +22,7 @@ import Combine
 @objc protocol MouseOverViewDelegate: AnyObject {
 
     @objc @MainActor optional func mouseOverView(_ mouseOverView: MouseOverView, isMouseOver: Bool)
+    @objc @MainActor optional func mouseOverView(_ mouseOverView: MouseOverView, mouseMovedWith: NSEvent)
 
     @objc @MainActor optional func mouseClickView(_ mouseClickView: MouseClickView, mouseDownEvent: NSEvent)
     @objc @MainActor optional func mouseClickView(_ mouseClickView: MouseClickView, mouseUpEvent: NSEvent)
@@ -150,6 +151,12 @@ internal class MouseOverView: NSControl, Hoverable {
         }
     }
 
+    override func mouseMoved(with event: NSEvent) {
+        super.mouseMoved(with: event)
+
+        delegate?.mouseOverView?(self, mouseMovedWith: event)
+    }
+
     override func mouseDown(with event: NSEvent) {
         guard isMouseLocationInsideBounds(event.locationInWindow) else { return }