From 51267d119362ff5b19dccdbb6564c42472dc7ce3 Mon Sep 17 00:00:00 2001 From: Alexey Martemyanov Date: Fri, 28 Oct 2022 16:45:04 +0700 Subject: [PATCH] Fix closing tabs quickly (#801) * fix closing tabs quickly * update evilHackToClearLastLeftHitInWindow * fix test * Fix mouse-exit events not handled correctly * minor adjustment --- DuckDuckGo.xcodeproj/project.pbxproj | 4 + .../Common/Extensions/NSViewExtension.swift | 2 +- .../Common/Extensions/NSWindowExtension.swift | 23 +++++ .../Common/View/AppKit/MouseClickView.swift | 28 +----- DuckDuckGo/Tab Bar/View/TabBarViewItem.swift | 6 ++ ...ppKitPrivateMethodsAvailabilityTests.swift | 87 +++++++++++++++++++ 6 files changed, 122 insertions(+), 28 deletions(-) create mode 100644 Unit Tests/Common/AppKitPrivateMethodsAvailabilityTests.swift diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index 5720d7e931..eec78a2ad1 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -709,6 +709,7 @@ B693956126F1C1BC0015B914 /* DownloadListStoreMock.swift in Sources */ = {isa = PBXBuildFile; fileRef = B693956026F1C1BC0015B914 /* DownloadListStoreMock.swift */; }; B693956326F1C2A40015B914 /* FileDownloadManagerMock.swift in Sources */ = {isa = PBXBuildFile; fileRef = B693956226F1C2A40015B914 /* FileDownloadManagerMock.swift */; }; B693956926F352DB0015B914 /* DownloadsWebViewMock.m in Sources */ = {isa = PBXBuildFile; fileRef = B693956826F352DB0015B914 /* DownloadsWebViewMock.m */; }; + B698E5042908011E00A746A8 /* AppKitPrivateMethodsAvailabilityTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B698E5032908011E00A746A8 /* AppKitPrivateMethodsAvailabilityTests.swift */; }; B69B503A2726A12500758A2B /* StatisticsLoader.swift in Sources */ = {isa = PBXBuildFile; fileRef = B69B50342726A11F00758A2B /* StatisticsLoader.swift */; }; B69B503B2726A12500758A2B /* Atb.swift in Sources */ = {isa = PBXBuildFile; fileRef = B69B50352726A11F00758A2B /* Atb.swift */; }; B69B503C2726A12500758A2B /* StatisticsStore.swift in Sources */ = {isa = PBXBuildFile; fileRef = B69B50362726A12000758A2B /* StatisticsStore.swift */; }; @@ -1555,6 +1556,7 @@ B693956626F352940015B914 /* TestsBridging.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestsBridging.h; sourceTree = ""; }; B693956726F352DB0015B914 /* DownloadsWebViewMock.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = DownloadsWebViewMock.h; sourceTree = ""; }; B693956826F352DB0015B914 /* DownloadsWebViewMock.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = DownloadsWebViewMock.m; sourceTree = ""; }; + B698E5032908011E00A746A8 /* AppKitPrivateMethodsAvailabilityTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppKitPrivateMethodsAvailabilityTests.swift; sourceTree = ""; }; B69B50342726A11F00758A2B /* StatisticsLoader.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = StatisticsLoader.swift; sourceTree = ""; }; B69B50352726A11F00758A2B /* Atb.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Atb.swift; sourceTree = ""; }; B69B50362726A12000758A2B /* StatisticsStore.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = StatisticsStore.swift; sourceTree = ""; }; @@ -2738,6 +2740,7 @@ 4BA1A6CE258BF58C00F6F690 /* File System */, B6AE74322609AFBB005B9B1A /* Progress */, 4B0511E6262CAB3700F6079C /* UserDefaultsWrapperUtilities.swift */, + B698E5032908011E00A746A8 /* AppKitPrivateMethodsAvailabilityTests.swift */, B6B3E0952654DACD0040E0A2 /* UTTypeTests.swift */, B693956626F352940015B914 /* TestsBridging.h */, ); @@ -5181,6 +5184,7 @@ 3767190028E58513003A2A15 /* PrivatePlayerURLExtensionTests.swift in Sources */, B6B3E0962654DACD0040E0A2 /* UTTypeTests.swift in Sources */, 4B2975992828285900187C4E /* FirefoxKeyReaderTests.swift in Sources */, + B698E5042908011E00A746A8 /* AppKitPrivateMethodsAvailabilityTests.swift in Sources */, 4B02199D25E063DE00ED7DEA /* FireproofingURLExtensionsTests.swift in Sources */, 4BB99D0F26FE1A84001E4761 /* ChromiumBookmarksReaderTests.swift in Sources */, 4BB99D1026FE1A84001E4761 /* FirefoxBookmarksReaderTests.swift in Sources */, diff --git a/DuckDuckGo/Common/Extensions/NSViewExtension.swift b/DuckDuckGo/Common/Extensions/NSViewExtension.swift index 54385b272d..b0f55ba94f 100644 --- a/DuckDuckGo/Common/Extensions/NSViewExtension.swift +++ b/DuckDuckGo/Common/Extensions/NSViewExtension.swift @@ -130,7 +130,7 @@ extension NSView { func mouseLocationInsideBounds(_ point: NSPoint?) -> NSPoint? { withMouseLocationInViewCoordinates(point) { locationInView in - guard self.bounds.contains(locationInView) else { return nil } + guard self.visibleRect.contains(locationInView) else { return nil } return locationInView } } diff --git a/DuckDuckGo/Common/Extensions/NSWindowExtension.swift b/DuckDuckGo/Common/Extensions/NSWindowExtension.swift index a4789c9325..30f243e4db 100644 --- a/DuckDuckGo/Common/Extensions/NSWindowExtension.swift +++ b/DuckDuckGo/Common/Extensions/NSWindowExtension.swift @@ -25,4 +25,27 @@ extension NSWindow { setFrameOrigin(frameOrigin) } + private static let lastLeftHitKey = "_lastLeftHit" + var lastLeftHit: NSView? { + return try? NSException.catch { + self.value(forKey: Self.lastLeftHitKey) as? NSView + } + } + + func evilHackToClearLastLeftHitInWindow() { + guard let oldValue = self.lastLeftHit else { return } + let oldValueRetainCount = CFGetRetainCount(oldValue) + defer { + // compensate unbalanced release call + if CFGetRetainCount(oldValue) < oldValueRetainCount { + _=Unmanaged.passUnretained(oldValue).retain() + } + } + NSException.try { + autoreleasepool { + self.setValue(nil, forKey: Self.lastLeftHitKey) + } + } + } + } diff --git a/DuckDuckGo/Common/View/AppKit/MouseClickView.swift b/DuckDuckGo/Common/View/AppKit/MouseClickView.swift index b0170d1aeb..9e9581c7ae 100644 --- a/DuckDuckGo/Common/View/AppKit/MouseClickView.swift +++ b/DuckDuckGo/Common/View/AppKit/MouseClickView.swift @@ -40,43 +40,19 @@ final class MouseClickView: NSView { weak var delegate: MouseClickViewDelegate? - private func repostMultiClickEventIfNeeded(_ event: NSEvent) -> Bool { - // don't let more-than-doubleclicks get in - guard event.clickCount > 2, - let newEvent = NSEvent.mouseEvent(with: event.type, - location: event.locationInWindow, - modifierFlags: event.modifierFlags, - timestamp: event.timestamp, - windowNumber: event.windowNumber, - context: nil, - eventNumber: event.eventNumber, - clickCount: 1, - pressure: event.pressure), - let window = self.window - else { return false } - - // break the growing clickCount event cycle by sending a new 1-click event to the window - window.postEvent(newEvent, atStart: true) - return true - } - override func mouseDown(with event: NSEvent) { let coordinateInWindow = event.locationInWindow let coordinateInView = self.convert(coordinateInWindow, from: nil) - + if !self.bounds.contains(coordinateInView) { return } - - guard !repostMultiClickEventIfNeeded(event) else { return } super.mouseDown(with: event) delegate?.mouseClickView(self, mouseDownEvent: event) } override func mouseUp(with event: NSEvent) { - guard !repostMultiClickEventIfNeeded(event) else { return } - super.mouseUp(with: event) delegate?.mouseClickView(self, mouseUpEvent: event) } @@ -88,8 +64,6 @@ final class MouseClickView: NSView { } override func otherMouseDown(with event: NSEvent) { - guard !repostMultiClickEventIfNeeded(event) else { return } - super.otherMouseDown(with: event) delegate?.mouseClickView(self, otherMouseDownEvent: event) diff --git a/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift b/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift index 78f6212a0e..e5d8f8aa1f 100644 --- a/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift +++ b/DuckDuckGo/Tab Bar/View/TabBarViewItem.swift @@ -176,6 +176,12 @@ final class TabBarViewItem: NSCollectionViewItem { private var lastKnownIndexPath: IndexPath? @IBAction func closeButtonAction(_ sender: NSButton) { + // due to async nature of NSCollectionView views removal + // leaving window._lastLeftHit set to the button will prevent + // continuous clicks on the Close button + // this should be removed when the Tab Bar is redone without NSCollectionView + sender.window?.evilHackToClearLastLeftHitInWindow() + guard let indexPath = self.collectionView?.indexPath(for: self) else { // doubleclick event arrived at point when we're already removed // pass the closeButton action to the next TabBarViewItem diff --git a/Unit Tests/Common/AppKitPrivateMethodsAvailabilityTests.swift b/Unit Tests/Common/AppKitPrivateMethodsAvailabilityTests.swift new file mode 100644 index 0000000000..cb1f6c9295 --- /dev/null +++ b/Unit Tests/Common/AppKitPrivateMethodsAvailabilityTests.swift @@ -0,0 +1,87 @@ +// +// AppKitPrivateMethodsAvailabilityTests.swift +// +// Copyright © 2022 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import XCTest +import AppKit +@testable import DuckDuckGo_Privacy_Browser + +final class AppKitPrivateMethodsAvailabilityTests: XCTestCase { + + func testLastLeftHitViewIsReleasedCorrectly() { + var window: NSWindow! + autoreleasepool { + window = NSWindow() + window.isReleasedWhenClosed = false + + let view = TestHitView(frame: NSRect(x: 0, y: 0, width: 100, height: 100)) + window.contentView = view + + window.setFrame(NSRect(x: 0, y: 0, width: 100, height: 123), display: false) + NSApp.activate(ignoringOtherApps: true) + + let didAppearExpectation = expectation(description: "view did appear") + window.makeKeyAndOrderFront(nil) + DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { + didAppearExpectation.fulfill() + } + waitForExpectations(timeout: 1.0) + + view.mouseDownExpectation = expectation(description: "mouseDown received") + + let event = NSEvent.mouseEvent(with: .leftMouseDown, + location: NSPoint(x: 50, y: 50), + modifierFlags: [], + timestamp: CACurrentMediaTime(), + windowNumber: window.windowNumber, + context: nil, + eventNumber: -22966, + clickCount: 1, + pressure: 1)! + window.postEvent(event, atStart: false) + + waitForExpectations(timeout: 0.1) + + // window should have lastLeftHit set to the clicked view after a click event + XCTAssertEqual(window.lastLeftHit, view) + window.evilHackToClearLastLeftHitInWindow() + XCTAssertEqual(window.lastLeftHit, nil) + + view.deinitExpectation = expectation(description: "deinit called") + window.close() + window = nil + } + + waitForExpectations(timeout: 0.5) + } + +} + +private class TestHitView: NSView { + var mouseDownExpectation: XCTestExpectation! + var deinitExpectation: XCTestExpectation! + + override func mouseDown(with event: NSEvent) { + mouseDownExpectation.fulfill() + super.mouseDown(with: event) + } + + deinit { + deinitExpectation.fulfill() + } + +}