Skip to content

Commit

Permalink
Address bar skipping keys input (#3361)
Browse files Browse the repository at this point in the history 
Task/Issue URL: https://app.asana.com/0/1207340338530322/1208166085652339/f
  • Loading branch information
@mallexxx
mallexxx authored Oct 21, 2024
1 parent 2cd2028 commit 687c1de
Show file tree
Hide file tree
Showing 7 changed files with 357 additions and 97 deletions.
16 changes: 16 additions & 0 deletions DuckDuckGo.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1816,6 +1816,8 @@
84537A092C99C203008723BC /* DeallocationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B6C2C9EE276081AB005B7F0A /* DeallocationTests.swift */; };
848648A12C76F4B20082282D /* BookmarksBarMenuViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */; };
848648A22C76F4B20082282D /* BookmarksBarMenuViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 848648A02C76F4B20082282D /* BookmarksBarMenuViewController.swift */; };
84B49F0D2CB10F0900FF08BB /* OHHTTPStubs in Frameworks */ = {isa = PBXBuildFile; productRef = 84B49F0C2CB10F0900FF08BB /* OHHTTPStubs */; };
84B49F0F2CB10F0900FF08BB /* OHHTTPStubsSwift in Frameworks */ = {isa = PBXBuildFile; productRef = 84B49F0E2CB10F0900FF08BB /* OHHTTPStubsSwift */; };
84DC715A2C1C1E9000033B8C /* UserDefaultsWrapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 84DC71582C1C1E8A00033B8C /* UserDefaultsWrapperTests.swift */; };
84DC715B2C1C1E9000033B8C /* UserDefaultsWrapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 84DC71582C1C1E8A00033B8C /* UserDefaultsWrapperTests.swift */; };
84DDB90A2C92B66E008C997B /* WKVisitedLinkStoreWrapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 84DDB9092C92B667008C997B /* WKVisitedLinkStoreWrapper.swift */; };
Expand Down Expand Up @@ -4751,6 +4753,8 @@
buildActionMask = 2147483647;
files = (
B65CD8D12B316E0C00A595BB /* SnapshotTesting in Frameworks */,
84B49F0F2CB10F0900FF08BB /* OHHTTPStubsSwift in Frameworks */,
84B49F0D2CB10F0900FF08BB /* OHHTTPStubs in Frameworks */,
1EB028332C91C754005343F6 /* Common in Frameworks */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down Expand Up @@ -9393,6 +9397,8 @@
packageProductDependencies = (
B65CD8D02B316E0C00A595BB /* SnapshotTesting */,
1EB028322C91C754005343F6 /* Common */,
84B49F0C2CB10F0900FF08BB /* OHHTTPStubs */,
84B49F0E2CB10F0900FF08BB /* OHHTTPStubsSwift */,
);
productName = "Integration Tests";
productReference = 3706FEB2293F662100E42796 /* Integration Tests App Store.xctest */;
Expand Down Expand Up @@ -14908,6 +14914,16 @@
isa = XCSwiftPackageProductDependency;
productName = NetworkProtectionUI;
};
84B49F0C2CB10F0900FF08BB /* OHHTTPStubs */ = {
isa = XCSwiftPackageProductDependency;
package = B6DA44152616C13800DD1EC2 /* XCRemoteSwiftPackageReference "OHHTTPStubs" */;
productName = OHHTTPStubs;
};
84B49F0E2CB10F0900FF08BB /* OHHTTPStubsSwift */ = {
isa = XCSwiftPackageProductDependency;
package = B6DA44152616C13800DD1EC2 /* XCRemoteSwiftPackageReference "OHHTTPStubs" */;
productName = OHHTTPStubsSwift;
};
85D44B852BA08D29001B4AB5 /* Suggestions */ = {
isa = XCSwiftPackageProductDependency;
package = 9807F643278CA16F00E1547B /* XCRemoteSwiftPackageReference "BrowserServicesKit" */;
Expand Down
23 changes: 11 additions & 12 deletions DuckDuckGo/NavigationBar/View/AddressBarTextField.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,8 @@ final class AddressBarTextField: NSTextField {

private func subscribeToSelectedSuggestionViewModel() {
selectedSuggestionViewModelCancellable = suggestionContainerViewModel?.$selectedSuggestionViewModel
.receive(on: DispatchQueue.main)
.sink { [weak self] _ in
self?.displaySelectedSuggestionViewModel()
.sink { [weak self] selectedSuggestionViewModel in
self?.displaySelectedSuggestionViewModel(selectedSuggestionViewModel)
}
}

Expand Down Expand Up @@ -500,14 +499,14 @@ final class AddressBarTextField: NSTextField {

// MARK: - Suggestion window

private func displaySelectedSuggestionViewModel() {
private func displaySelectedSuggestionViewModel(_ selectedSuggestionViewModel: SuggestionViewModel?) {
guard let suggestionWindow = suggestionWindowController?.window else {
Logger.general.error("AddressBarTextField: Window not available")
return
}
guard suggestionWindow.isVisible else { return }

guard let selectedSuggestionViewModel = suggestionContainerViewModel?.selectedSuggestionViewModel else {
guard let selectedSuggestionViewModel else {
if let originalStringValue = suggestionContainerViewModel?.userStringValue {
self.value = Value(stringValue: originalStringValue, userTyped: true)
} else {
Expand Down Expand Up @@ -909,6 +908,13 @@ extension AddressBarTextField: NSTextFieldDelegate {
self.currentTextDidChangeEvent = .none
}

if stringValue.isEmpty {
suggestionContainerViewModel?.clearUserStringValue()
hideSuggestionWindow()
} else {
suggestionContainerViewModel?.setUserStringValue(stringValueWithoutSuffix, userAppendedStringToTheEnd: currentTextDidChangeEvent == .userAppendingTextToTheEnd)
}

// if user continues typing letters from displayed Suggestion
// don't blink and keep the Suggestion displayed
if case .userAppendingTextToTheEnd = currentTextDidChangeEvent,
Expand All @@ -919,13 +925,6 @@ extension AddressBarTextField: NSTextFieldDelegate {
suggestionContainerViewModel?.clearSelection()
self.value = Value(stringValue: stringValueWithoutSuffix, userTyped: true)
}

if stringValue.isEmpty {
suggestionContainerViewModel?.clearUserStringValue()
hideSuggestionWindow()
} else {
suggestionContainerViewModel?.setUserStringValue(stringValueWithoutSuffix, userAppendedStringToTheEnd: currentTextDidChangeEvent == .userAppendingTextToTheEnd)
}
}

private func autocompleteSuggestionBeingTypedOverByUser(with newUserEnteredValue: String) -> SuggestionViewModel? {
Expand Down
10 changes: 5 additions & 5 deletions DuckDuckGo/Suggestions/Model/SuggestionContainer.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class SuggestionContainer {

static let maximumNumberOfSuggestions = 9

@Published private(set) var result: SuggestionResult?
@PublishedAfter private(set) var result: SuggestionResult?

private let historyCoordinating: HistoryCoordinating
private let bookmarkManager: BookmarkManager
Expand Down Expand Up @@ -60,9 +60,9 @@ final class SuggestionContainer {
loading.getSuggestions(query: query, usingDataSource: self) { [weak self] result, error in
dispatchPrecondition(condition: .onQueue(.main))

guard self?.latestQuery == query else { return }
guard let result = result else {
self?.result = nil
guard let self, self.latestQuery == query else { return }
guard let result else {
self.result = nil
Logger.general.error("Suggestions: Failed to get suggestions - \(String(describing: error))")
PixelKit.fire(DebugEvent(GeneralPixel.suggestionsFetchFailed, error: error))
return
Expand All @@ -73,7 +73,7 @@ final class SuggestionContainer {
Logger.general.error("Suggestions: Error when getting suggestions - \(error.localizedDescription)")
}

self?.result = result
self.result = result
}
}

Expand Down
61 changes: 36 additions & 25 deletions DuckDuckGo/Suggestions/ViewModel/SuggestionContainerViewModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
// limitations under the License.
//

import Foundation
import BrowserServicesKit
import Combine
import Common
import BrowserServicesKit
import Foundation
import os.log
import Suggestions

final class SuggestionContainerViewModel {

Expand Down Expand Up @@ -50,35 +51,45 @@ final class SuggestionContainerViewModel {

private var isTopSuggestionSelectionExpected = false

private var shouldSelectTopSuggestion: Bool {
guard let result = suggestionContainer.result, !result.isEmpty else { return false }

if self.isTopSuggestionSelectionExpected,
result.canBeAutocompleted,
let userStringValue = self.userStringValue,
let firstSuggestion = self.suggestionViewModel(at: 0),
firstSuggestion.autocompletionString.lowercased().hasPrefix(userStringValue.lowercased()) {
return true
} else {
return false
private enum IgnoreTopSuggestionError: Error {
case emptyResult
case topSuggestionSelectionNotExpected
case cantBeAutocompleted
case noUserStringValue
case noSuggestionViewModel
case notEqual(lhs: String, rhs: String)
}
private func validateShouldSelectTopSuggestion(from result: SuggestionResult?) throws {
assert(suggestionContainer.result == result)
guard let result, !result.isEmpty else { throw IgnoreTopSuggestionError.emptyResult }
guard self.isTopSuggestionSelectionExpected else { throw IgnoreTopSuggestionError.topSuggestionSelectionNotExpected }
guard result.canBeAutocompleted else {
throw IgnoreTopSuggestionError.cantBeAutocompleted
}
guard let userStringValue else { throw IgnoreTopSuggestionError.noUserStringValue }
guard let firstSuggestion = self.suggestionViewModel(at: 0) else { throw IgnoreTopSuggestionError.noSuggestionViewModel }
guard firstSuggestion.autocompletionString.lowercased().hasPrefix(userStringValue.lowercased()) else {
throw IgnoreTopSuggestionError.notEqual(lhs: firstSuggestion.autocompletionString, rhs: userStringValue)
}
}

private func subscribeToSuggestionResult() {
suggestionResultCancellable = suggestionContainer.$result.receive(on: DispatchQueue.main)
.sink { [weak self] _ in
guard let self = self,
self.shouldSelectTopSuggestion
else { return }

self.select(at: 0)
}
suggestionResultCancellable = suggestionContainer.$result
.sink { [weak self] result in
guard let self else { return }
do {
try validateShouldSelectTopSuggestion(from: result)
} catch {
Logger.general.debug("SuggestionContainerViewModel: ignoring top suggestion from \( result.map(String.init(describing:)) ?? "<nil>"): \(error)")
return
}

self.select(at: 0)
}
}

func setUserStringValue(_ userStringValue: String, userAppendedStringToTheEnd: Bool) {
guard SearchPreferences.shared.showAutocompleteSuggestions else {
return
}
guard SearchPreferences.shared.showAutocompleteSuggestions else { return }

let oldValue = self.userStringValue
self.userStringValue = userStringValue
Expand All @@ -100,7 +111,7 @@ final class SuggestionContainerViewModel {
}

private func updateSelectedSuggestionViewModel() {
if let selectionIndex = selectionIndex {
if let selectionIndex {
selectedSuggestionViewModel = suggestionViewModel(at: selectionIndex)
} else {
selectedSuggestionViewModel = nil
Expand Down
1 change: 1 addition & 0 deletions DuckDuckGo/Tab/TabExtensions/HistoryTabExtension.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ extension HistoryTabExtension: NavigationResponder {
func didCommit(_ navigation: Navigation) {
guard navigation.url == self.url,
navigation.url.isHypertextURL,
navigation.navigationAction.navigationType != .alternateHtmlLoad, // should not be loading error page
case .expected = visitState else { return }

guard !navigation.navigationAction.navigationType.isBackForward else {
Expand Down
62 changes: 62 additions & 0 deletions IntegrationTests/Tab/AddressBarTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@
import Carbon
import Combine
import Foundation
import History
import OHHTTPStubs
import OHHTTPStubsSwift
import Suggestions
import XCTest

@testable import DuckDuckGo_Privacy_Browser

@available(macOS 12.0, *)
Expand Down Expand Up @@ -101,6 +106,8 @@ class AddressBarTests: XCTestCase {

NSError.disableSwizzledDescription = false
StartupPreferences.shared.launchToCustomHomePage = false

HTTPStubs.removeAllStubs()
}

let asciiToCGEventMap: [String: UInt16] = [
Expand Down Expand Up @@ -190,6 +197,60 @@ class AddressBarTests: XCTestCase {

}

@MainActor
func testWhenAddressIsTyped_LoadedTopHitSuggestionIsCorrectlyAppendedAndSelected() {
// top hits should only work for visited urls
HistoryCoordinator.shared.addVisit(of: URL(string: "https://youtube.com")!)
let tab = Tab(content: .newtab)
window = WindowsManager.openNewWindow(with: tab)!

let json = """
[
{ "phrase": "youtube.com", "isNav": true },
{ "phrase": "ducks", "isNav": false },
]
"""
let address = "youtube.com"
stub {
$0.url!.absoluteString.hasPrefix("https://duckduckgo.com/ac/")
} response: { _ in
return HTTPStubsResponse(data: json.utf8data, statusCode: 200, headers: nil)
}

// This test reproduces a race condition where the $selectedSuggestionViewModel is published
// asynchronously on the main queue in response to user input. This can lead to a situation
// where the user-entered text in the suggestion model is one letter shorter than the actual
// user input in the address field.
// As a result, an extra letter may be selected and overtyped, causing a "skipped
// letter" effect or typographical errors. The goal of this test is to verify that the
// suggestion model correctly reflects the user's input without any discrepancies.
// https://app.asana.com/0/1207340338530322/1208166085652339/f
//
// Here we simulate receiving the keyboard event right after the suggestion view model received event.
var index = address.startIndex
let e = expectation(description: "typing done")
let c = addressBarTextField.suggestionContainerViewModel!.$selectedSuggestionViewModel
.receive(on: DispatchQueue.main)
.sink { [unowned self] suggestion in
guard suggestion != nil /* suggestion shown */
|| index == address.startIndex /* address is empty (first iteration) */ else {

return
}
guard index < address.endIndex else {
e.fulfill() // typing done
return
}
type(String(address[index]))
index = address.index(after: index)
}

waitForExpectations(timeout: 5)
withExtendedLifetime(c) {}

XCTAssertEqual("youtube.com", addressBarValue.prefix("youtube.com".count))
}

@MainActor
func testWhenSwitchingBetweenTabs_addressBarFocusStateIsCorrect() async throws {
let viewModel = TabCollectionViewModel(tabCollection: TabCollection(tabs: [
Expand Down Expand Up @@ -896,6 +957,7 @@ class AddressBarTests: XCTestCase {
let zoomButton = mainViewController.navigationBarViewController.addressBarViewController!.addressBarButtonsViewController!.zoomButton!
XCTAssertTrue(zoomButton.isHidden)
}

}

protocol MainActorPerformer {
Expand Down
Loading

0 comments on commit 687c1de

Please sign in to comment.