Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Commit 0467ca2

Browse files
mallexxxmallexxx
authored
Navigation: Improve same-document navigation handling (#2334)
Task/Issue URL: https://app.asana.com/0/1199230911884351/1206765227440723/f BSK PR: duckduckgo/BrowserServicesKit#702
1 parent a88bebf commit 0467ca2

File tree

12 files changed

+86
-35
lines changed

12 files changed

+86
-35
lines changed

‎DuckDuckGo.xcodeproj/project.pbxproj

+1-1
Original file line numberDiff line numberDiff line change
@@ -13693,7 +13693,7 @@
1369313693
repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
1369413694
requirement = {
1369513695
kind = exactVersion;
13696-
version = 118.0.0;
13696+
version = 119.0.0;
1369713697
};
1369813698
};
1369913699
AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {

‎DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

+3-3
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@
2323
"kind" : "remoteSourceControl",
2424
"location" : "https://github.com/duckduckgo/BrowserServicesKit",
2525
"state" : {
26-
"revision" : "2181cdcd27be961f10d988fbb202431a6ec6d56d",
27-
"version" : "118.0.0"
26+
"revision" : "4504b1452d059e5a87220b402d939a33d33d07dc",
27+
"version" : "119.0.0"
2828
}
2929
},
3030
{
@@ -156,7 +156,7 @@
156156
{
157157
"identity" : "trackerradarkit",
158158
"kind" : "remoteSourceControl",
159-
"location" : "https://github.com/duckduckgo/TrackerRadarKit",
159+
"location" : "https://github.com/duckduckgo/TrackerRadarKit.git",
160160
"state" : {
161161
"revision" : "a6b7ba151d9dc6684484f3785293875ec01cc1ff",
162162
"version" : "1.2.2"

‎DuckDuckGo/Tab/Model/Tab+Navigation.swift

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@
1616
// limitations under the License.
1717
//
1818

19-
import Navigation
19+
import Combine
2020
import Common
2121
import Foundation
22+
import Navigation
2223
import WebKit
2324

2425
extension Tab: NavigationResponder {

‎DuckDuckGo/Tab/Model/Tab.swift

+27-5
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,9 @@ protocol NewWindowPolicyDecisionMaker {
622622
let webViewDidStartNavigationPublisher = PassthroughSubject<Void, Never>()
623623
let webViewDidReceiveUserInteractiveChallengePublisher = PassthroughSubject<Void, Never>()
624624
let webViewDidReceiveRedirectPublisher = PassthroughSubject<Void, Never>()
625-
let webViewDidCommitNavigationPublisher = PassthroughSubject<Void, Never>()
625+
var webViewDidCommitNavigationPublisher: some Publisher<Void, Never> {
626+
$committedURL.dropFirst().asVoid()
627+
}
626628
let webViewDidFinishNavigationPublisher = PassthroughSubject<Void, Never>()
627629

628630
// MARK: - Properties
@@ -648,6 +650,13 @@ protocol NewWindowPolicyDecisionMaker {
648650
}
649651
}
650652

653+
/// Use this property to obtain the accurate URL of the displayed content
654+
///
655+
/// When a navigation request is made, the web view goes through a series of steps to load and display the requested content.
656+
/// The committedURL property reflects the URL of the web page that has undergone this process and is currently being displayed in the web view.
657+
/// Navigations that are still in progress or not yet committed won't have their URLs reflected in the committedURL property.
658+
@PublishedAfter private(set) var committedURL: URL?
659+
651660
@discardableResult
652661
func setContent(_ newContent: TabContent) -> ExpectedNavigation? {
653662
guard contentChangeEnabled else { return nil }
@@ -690,6 +699,8 @@ protocol NewWindowPolicyDecisionMaker {
690699
self.content = content
691700
}
692701
} else if self.content.isUrl {
702+
// when e.g. opening a download in new tab - web view restores `nil` after the navigation is interrupted
703+
// maybe it worths adding another content type like .interruptedLoad(URL) to display a URL in the address bar
693704
self.content = .none
694705
}
695706
self.updateTitle() // The title might not change if webView doesn't think anything is different so update title here as well
@@ -1304,7 +1315,7 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
13041315

13051316
@MainActor
13061317
func didCommit(_ navigation: Navigation) {
1307-
webViewDidCommitNavigationPublisher.send()
1318+
committedURL = navigation.url
13081319
}
13091320

13101321
@MainActor
@@ -1383,12 +1394,23 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
13831394
}
13841395

13851396
@MainActor
1386-
func navigation(_ navigation: Navigation, didFailWith error: WKError) {
1397+
func navigation(_ navigation: Navigation, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType) {
1398+
guard navigation.isCurrent else { return }
1399+
13871400
invalidateInteractionStateData()
1401+
if navigation.url.host == committedURL?.host {
1402+
committedURL = navigation.url
1403+
}
1404+
}
13881405

1406+
@MainActor
1407+
func navigation(_ navigation: Navigation, didFailWith error: WKError) {
13891408
let url = error.failingUrl ?? navigation.url
1390-
if navigation.isCurrent,
1391-
!error.isFrameLoadInterrupted, !error.isNavigationCancelled,
1409+
guard navigation.isCurrent else { return }
1410+
1411+
invalidateInteractionStateData()
1412+
1413+
if !error.isFrameLoadInterrupted, !error.isNavigationCancelled,
13921414
// don‘t show an error page if the error was already handled
13931415
// (by SearchNonexistentDomainNavigationResponder) or another navigation was triggered by `setContent`
13941416
self.content.urlForWebView == url {

‎DuckDuckGo/Tab/TabExtensions/DuckPlayerTabExtension.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ extension DuckPlayerTabExtension: NavigationResponder {
204204
return .next
205205
}
206206

207-
func navigation(_ navigation: Navigation?, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType?) {
207+
func navigation(_ navigation: Navigation, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType) {
208208
// Navigating to a Youtube URL without page reload
209209
if duckPlayer.mode == .enabled,
210210
case .sessionStatePush = navigationType,

‎DuckDuckGo/Tab/TabExtensions/FindInPageTabExtension.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ extension FindInPageTabExtension: NavigationResponder {
218218
close()
219219
}
220220

221-
func navigation(_ navigation: Navigation?, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType?) {
221+
func navigation(_ navigation: Navigation, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType) {
222222
if case .sessionStateReplace = navigationType {
223223
close()
224224
}

‎DuckDuckGo/Tab/TabExtensions/HistoryTabExtension.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,8 @@ extension HistoryTabExtension: NavigationResponder {
204204
addVisit()
205205
}
206206

207-
func willStart(_ navigation: Navigation) {
208-
if case .sameDocumentNavigation = navigation.navigationAction.navigationType {
207+
func navigation(_ navigation: Navigation, didSameDocumentNavigationOf navigationType: WKSameDocumentNavigationType) {
208+
if navigation.isCurrent, navigationType != .sessionStatePop {
209209
self.url = navigation.navigationAction.url
210210
addVisit()
211211
}

‎DuckDuckGo/Tab/ViewModel/TabViewModel.swift

+10-5
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@
1616
// limitations under the License.
1717
//
1818

19+
import BrowserServicesKit
1920
import Cocoa
2021
import Combine
21-
import BrowserServicesKit
22+
import Common
2223

2324
final class TabViewModel {
2425

@@ -108,15 +109,19 @@ final class TabViewModel {
108109
tab.$content
109110
.map { [tab] content -> AnyPublisher<Event, Never> in
110111
switch content {
111-
case .url(_, _, source: .webViewUpdated),
112-
.url(_, _, source: .link):
112+
case .url(let url, _, source: .webViewUpdated),
113+
.url(let url, _, source: .link):
113114

114115
// Update the address bar only after the tab did commit navigation to prevent Address Bar Spoofing
115-
return tab.webViewDidCommitNavigationPublisher.map { .didCommit }.eraseToAnyPublisher()
116+
return tab.$committedURL.filter { committedURL in
117+
committedURL == url
118+
}.map { _ in
119+
.didCommit
120+
}.eraseToAnyPublisher()
116121

117122
case .url(_, _, source: .userEntered(_, downloadRequested: true)):
118123
// don‘t update the address bar for download navigations
119-
return Empty().eraseToAnyPublisher().eraseToAnyPublisher()
124+
return Empty().eraseToAnyPublisher()
120125

121126
case .url(_, _, source: .pendingStateRestoration),
122127
.url(_, _, source: .loadedByStateRestoration),

‎LocalPackages/DataBrokerProtection/Package.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ let package = Package(
2929
targets: ["DataBrokerProtection"])
3030
],
3131
dependencies: [
32-
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "118.0.0"),
32+
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "119.0.0"),
3333
.package(path: "../PixelKit"),
3434
.package(path: "../SwiftUIExtensions"),
3535
.package(path: "../XPCHelper"),

‎LocalPackages/NetworkProtectionMac/Package.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ let package = Package(
3131
.library(name: "NetworkProtectionUI", targets: ["NetworkProtectionUI"])
3232
],
3333
dependencies: [
34-
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "118.0.0"),
34+
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "119.0.0"),
3535
.package(path: "../XPCHelper"),
3636
.package(path: "../SwiftUIExtensions"),
3737
.package(path: "../LoginItems"),

‎LocalPackages/SubscriptionUI/Package.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ let package = Package(
1212
targets: ["SubscriptionUI"]),
1313
],
1414
dependencies: [
15-
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "118.0.0"),
15+
.package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "119.0.0"),
1616
.package(path: "../SwiftUIExtensions")
1717
],
1818
targets: [

‎UnitTests/Tab/ViewModel/TabViewModelTests.swift

+36-13
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,26 @@
1616
// limitations under the License.
1717
//
1818

19-
import XCTest
2019
import Combine
20+
import Navigation
21+
import XCTest
22+
2123
@testable import DuckDuckGo_Privacy_Browser
2224

23-
@MainActor
2425
final class TabViewModelTests: XCTestCase {
2526

2627
var cancellables = Set<AnyCancellable>()
2728

2829
// MARK: - Can reload
2930

31+
@MainActor
3032
func testWhenURLIsNilThenCanReloadIsFalse() {
3133
let tabViewModel = TabViewModel.aTabViewModel
3234

3335
XCTAssertFalse(tabViewModel.canReload)
3436
}
3537

38+
@MainActor
3639
func testWhenURLIsNotNilThenCanReloadIsTrue() {
3740
let tabViewModel = TabViewModel.forTabWithURL(.duckDuckGo)
3841

@@ -46,19 +49,22 @@ final class TabViewModelTests: XCTestCase {
4649

4750
// MARK: - AddressBarString
4851

52+
@MainActor
4953
func testWhenURLIsNilThenAddressBarStringIsEmpty() {
5054
let tabViewModel = TabViewModel.aTabViewModel
5155

5256
XCTAssertEqual(tabViewModel.addressBarString, "")
5357
}
5458

59+
@MainActor
5560
func testWhenURLIsSetThenAddressBarIsUpdated() {
5661
let urlString = "http://spreadprivacy.com"
57-
let tabViewModel = TabViewModel.forTabWithURL(.makeURL(from: urlString)!)
62+
let url = URL.makeURL(from: urlString)!
63+
let tabViewModel = TabViewModel.forTabWithURL(url)
5864

5965
let addressBarStringExpectation = expectation(description: "Address bar string")
6066

61-
tabViewModel.simulateLoadingCompletion()
67+
tabViewModel.simulateLoadingCompletion(url, in: tabViewModel.tab.webView)
6268

6369
tabViewModel.$addressBarString.debounce(for: 0.5, scheduler: RunLoop.main).sink { _ in
6470
XCTAssertEqual(tabViewModel.addressBarString, urlString)
@@ -67,13 +73,15 @@ final class TabViewModelTests: XCTestCase {
6773
waitForExpectations(timeout: 1, handler: nil)
6874
}
6975

76+
@MainActor
7077
func testWhenURLIsFileURLThenAddressBarIsFilePath() {
7178
let urlString = "file:///Users/Dax/file.txt"
72-
let tabViewModel = TabViewModel.forTabWithURL(.makeURL(from: urlString)!)
79+
let url = URL.makeURL(from: urlString)!
80+
let tabViewModel = TabViewModel.forTabWithURL(url)
7381

7482
let addressBarStringExpectation = expectation(description: "Address bar string")
7583

76-
tabViewModel.simulateLoadingCompletion()
84+
tabViewModel.simulateLoadingCompletion(url, in: tabViewModel.tab.webView)
7785

7886
tabViewModel.$addressBarString.debounce(for: 0.1, scheduler: RunLoop.main).sink { _ in
7987
XCTAssertEqual(tabViewModel.addressBarString, urlString)
@@ -83,13 +91,15 @@ final class TabViewModelTests: XCTestCase {
8391
waitForExpectations(timeout: 1, handler: nil)
8492
}
8593

94+
@MainActor
8695
func testWhenURLIsDataURLThenAddressBarIsDataURL() {
8796
let urlString = "data:,Hello%2C%20World%21"
88-
let tabViewModel = TabViewModel.forTabWithURL(.makeURL(from: urlString)!)
97+
let url = URL.makeURL(from: urlString)!
98+
let tabViewModel = TabViewModel.forTabWithURL(url)
8999

90100
let addressBarStringExpectation = expectation(description: "Address bar string")
91101

92-
tabViewModel.simulateLoadingCompletion()
102+
tabViewModel.simulateLoadingCompletion(url, in: tabViewModel.tab.webView)
93103

94104
tabViewModel.$addressBarString.debounce(for: 0.1, scheduler: RunLoop.main).sink { _ in
95105
XCTAssertEqual(tabViewModel.addressBarString, urlString)
@@ -99,6 +109,7 @@ final class TabViewModelTests: XCTestCase {
99109
waitForExpectations(timeout: 1, handler: nil)
100110
}
101111

112+
@MainActor
102113
func testWhenURLIsBlobURLWithBasicAuthThenAddressBarStripsBasicAuth() {
103114
let urlStrings = ["blob:https://spoofed.domain.com%20%20%20%20%20%20%20%20%20@attacker.com",
104115
"blob:ftp://another.spoofed.domain.com%20%20%20%20%20%20%20%20%20@attacker.com",
@@ -109,9 +120,10 @@ final class TabViewModelTests: XCTestCase {
109120
let uuidRegex = try! NSRegularExpression(pattern: uuidPattern, options: [])
110121

111122
for i in 0..<urlStrings.count {
112-
let tabViewModel = TabViewModel.forTabWithURL(.makeURL(from: urlStrings[i])!)
123+
let url = URL.makeURL(from: urlStrings[i])!
124+
let tabViewModel = TabViewModel.forTabWithURL(url)
113125
let addressBarStringExpectation = expectation(description: "Address bar string")
114-
tabViewModel.simulateLoadingCompletion()
126+
tabViewModel.simulateLoadingCompletion(url, in: tabViewModel.tab.webView)
115127

116128
tabViewModel.$addressBarString.debounce(for: 0.1, scheduler: RunLoop.main).sink { _ in
117129
XCTAssertTrue(tabViewModel.addressBarString.starts(with: expectedStarts[i]))
@@ -128,12 +140,14 @@ final class TabViewModelTests: XCTestCase {
128140

129141
// MARK: - Title
130142

143+
@MainActor
131144
func testWhenURLIsNilThenTitleIsNewTab() {
132145
let tabViewModel = TabViewModel.aTabViewModel
133146

134-
XCTAssertEqual(tabViewModel.title, "New Tab")
147+
XCTAssertEqual(tabViewModel.title, UserText.tabHomeTitle)
135148
}
136149

150+
@MainActor
137151
func testWhenTabTitleIsNotNilThenTitleReflectsTabTitle() async throws {
138152
let tabViewModel = TabViewModel.forTabWithURL(.duckDuckGo)
139153
let testTitle = "Test title"
@@ -153,6 +167,7 @@ final class TabViewModelTests: XCTestCase {
153167
await fulfillment(of: [titleExpectation], timeout: 0.5)
154168
}
155169

170+
@MainActor
156171
func testWhenTabTitleIsNilThenTitleIsAddressBarString() {
157172
let tabViewModel = TabViewModel.forTabWithURL(.duckDuckGo)
158173

@@ -167,13 +182,15 @@ final class TabViewModelTests: XCTestCase {
167182

168183
// MARK: - Favicon
169184

185+
@MainActor
170186
func testWhenContentIsNoneThenFaviconIsNil() {
171187
let tab = Tab(content: .none)
172188
let tabViewModel = TabViewModel(tab: tab)
173189

174190
XCTAssertEqual(tabViewModel.favicon, nil)
175191
}
176192

193+
@MainActor
177194
func testWhenContentIsHomeThenFaviconIsHome() {
178195
let tabViewModel = TabViewModel.aTabViewModel
179196
tabViewModel.tab.setContent(.newtab)
@@ -194,13 +211,15 @@ final class TabViewModelTests: XCTestCase {
194211

195212
// MARK: - Zoom
196213

214+
@MainActor
197215
func testThatDefaultValueForTabsWebViewIsOne() {
198216
UserDefaultsWrapper<Any>.clearAll()
199217
let tabVM = TabViewModel(tab: Tab(), appearancePreferences: AppearancePreferences())
200218

201219
XCTAssertEqual(tabVM.tab.webView.zoomLevel, DefaultZoomValue.percent100)
202220
}
203221

222+
@MainActor
204223
func testWhenAppearancePreferencesZoomLevelIsSetThenTabsWebViewZoomLevelIsUpdated() {
205224
UserDefaultsWrapper<Any>.clearAll()
206225
let tabVM = TabViewModel(tab: Tab())
@@ -210,6 +229,7 @@ final class TabViewModelTests: XCTestCase {
210229
XCTAssertEqual(tabVM.tab.webView.zoomLevel, randomZoomLevel)
211230
}
212231

232+
@MainActor
213233
func testWhenAppearancePreferencesZoomLevelIsSetAndANewTabIsOpenThenItsWebViewHasTheLatestValueOfZoomLevel() {
214234
UserDefaultsWrapper<Any>.clearAll()
215235
let randomZoomLevel = DefaultZoomValue.allCases.randomElement()!
@@ -236,8 +256,11 @@ extension TabViewModel {
236256
return TabViewModel(tab: tab)
237257
}
238258

239-
func simulateLoadingCompletion() {
240-
self.tab.webViewDidCommitNavigationPublisher.send(())
259+
@MainActor
260+
func simulateLoadingCompletion(_ url: URL, in webView: WKWebView) {
261+
let navAction = NavigationAction(request: URLRequest(url: url), navigationType: .other, currentHistoryItemIdentity: nil, redirectHistory: nil, isUserInitiated: nil, sourceFrame: .mainFrame(for: webView), targetFrame: .mainFrame(for: webView), shouldDownload: false, mainFrameNavigation: nil)
262+
let navigation = Navigation(identity: .init(nil), responders: .init(), state: .started, redirectHistory: [navAction], isCurrent: true, isCommitted: true)
263+
self.tab.didCommit(navigation)
241264
}
242265

243266
}

0 commit comments

Comments
 (0)