Fix browser back navigation history handoff (#1897)
* Add regression test for browser back history * Fix browser back history handoff * Fix browser tab favicon not updating on navigation Two issues caused stale or missing favicons in browser tabs: 1. KVO race: The isLoading observer read webView.isLoading inside a deferred Task instead of capturing the KVO change value at observation time. For fast navigations (back-forward cache), isLoading flips true→false before the Task runs, so handleWebViewLoadingChanged(true) was never called and the old favicon was never cleared. 2. SPA favicon discovery: Sites that inject <link rel="icon"> via JavaScript (e.g. React apps) had no favicon link in the DOM when didFinish fired. The fallback to /favicon.ico often 404'd, leaving the globe icon permanently. Now retries the JS query after 600ms to give client-side scripts time to add the tag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Austin Wang
GitHub
parent
5c4fab1296
commit
cdf8d367b2
2 changed files with 237 additions and 20 deletions
|
|
@ -2525,6 +2525,7 @@ final class BrowserPanel: Panel, ObservableObject {
|
|||
navigationDelegate.didFinish = { [weak self] webView in
|
||||
Task { @MainActor [weak self] in
|
||||
guard let self, self.isCurrentWebView(webView, instanceID: boundWebViewInstanceID) else { return }
|
||||
self.realignRestoredSessionHistoryToLiveCurrentIfPossible()
|
||||
boundHistoryStore.recordVisit(url: webView.url, title: webView.title)
|
||||
self.refreshFavicon(from: webView)
|
||||
self.applyBrowserThemeModeIfNeeded()
|
||||
|
|
@ -2867,20 +2868,109 @@ final class BrowserPanel: Panel, ObservableObject {
|
|||
backHistoryURLStrings: [String],
|
||||
forwardHistoryURLStrings: [String]
|
||||
) {
|
||||
realignRestoredSessionHistoryToLiveCurrentIfPossible()
|
||||
|
||||
let nativeBack = webView.backForwardList.backList.compactMap {
|
||||
Self.serializableSessionHistoryURLString($0.url)
|
||||
}
|
||||
let nativeForward = webView.backForwardList.forwardList.compactMap {
|
||||
Self.serializableSessionHistoryURLString($0.url)
|
||||
}
|
||||
|
||||
if usesRestoredSessionHistory {
|
||||
let back = restoredBackHistoryStack.compactMap { Self.serializableSessionHistoryURLString($0) }
|
||||
// `restoredForwardHistoryStack` stores nearest-forward entries at the end.
|
||||
let forward = restoredForwardHistoryStack.reversed().compactMap { Self.serializableSessionHistoryURLString($0) }
|
||||
return (back, forward)
|
||||
let restoredForward = restoredForwardHistoryStack.reversed().compactMap {
|
||||
Self.serializableSessionHistoryURLString($0)
|
||||
}
|
||||
|
||||
if isLiveSessionHistoryAlignedWithRestoredCurrent {
|
||||
return (
|
||||
back,
|
||||
restoredForward.isEmpty ? nativeForward : restoredForward
|
||||
)
|
||||
}
|
||||
|
||||
return (back + nativeBack, nativeForward)
|
||||
}
|
||||
|
||||
let back = webView.backForwardList.backList.compactMap {
|
||||
Self.serializableSessionHistoryURLString($0.url)
|
||||
return (nativeBack, nativeForward)
|
||||
}
|
||||
|
||||
private func resolvedLiveSessionHistoryURL() -> URL? {
|
||||
if let webViewURL = Self.remoteProxyDisplayURL(for: webView.url),
|
||||
Self.serializableSessionHistoryURLString(webViewURL) != nil {
|
||||
return webViewURL
|
||||
}
|
||||
let forward = webView.backForwardList.forwardList.compactMap {
|
||||
Self.serializableSessionHistoryURLString($0.url)
|
||||
if let currentURL,
|
||||
Self.serializableSessionHistoryURLString(currentURL) != nil {
|
||||
return currentURL
|
||||
}
|
||||
return (back, forward)
|
||||
return nil
|
||||
}
|
||||
|
||||
private var isLiveSessionHistoryAlignedWithRestoredCurrent: Bool {
|
||||
let liveCurrent = Self.serializableSessionHistoryURLString(resolvedLiveSessionHistoryURL())
|
||||
let restoredCurrent = Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL)
|
||||
guard let liveCurrent, let restoredCurrent else { return true }
|
||||
return liveCurrent == restoredCurrent
|
||||
}
|
||||
|
||||
private func realignRestoredSessionHistoryToLiveCurrentIfPossible() {
|
||||
guard usesRestoredSessionHistory else { return }
|
||||
guard let liveCurrent = resolvedLiveSessionHistoryURL(),
|
||||
let liveCurrentString = Self.serializableSessionHistoryURLString(liveCurrent) else {
|
||||
return
|
||||
}
|
||||
guard Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL) != liveCurrentString else {
|
||||
return
|
||||
}
|
||||
|
||||
let restoredBack = restoredBackHistoryStack.compactMap { Self.serializableSessionHistoryURLString($0) }
|
||||
let restoredForward = restoredForwardHistoryStack.reversed().compactMap {
|
||||
Self.serializableSessionHistoryURLString($0)
|
||||
}
|
||||
let restoredCurrent = Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL)
|
||||
|
||||
if let backIndex = restoredBack.lastIndex(of: liveCurrentString) {
|
||||
let newBack = Array(restoredBack[..<backIndex])
|
||||
var newForward = Array(restoredBack[(backIndex + 1)...])
|
||||
if let restoredCurrent {
|
||||
newForward.append(restoredCurrent)
|
||||
}
|
||||
newForward.append(contentsOf: restoredForward)
|
||||
|
||||
restoredBackHistoryStack = Self.sanitizedSessionHistoryURLs(newBack)
|
||||
restoredForwardHistoryStack = Array(Self.sanitizedSessionHistoryURLs(newForward).reversed())
|
||||
restoredHistoryCurrentURL = liveCurrent
|
||||
refreshNavigationAvailability()
|
||||
return
|
||||
}
|
||||
|
||||
if let forwardIndex = restoredForward.firstIndex(of: liveCurrentString) {
|
||||
var newBack = restoredBack
|
||||
if let restoredCurrent {
|
||||
newBack.append(restoredCurrent)
|
||||
}
|
||||
newBack.append(contentsOf: restoredForward[..<forwardIndex])
|
||||
let newForward = Array(restoredForward[(forwardIndex + 1)...])
|
||||
|
||||
restoredBackHistoryStack = Self.sanitizedSessionHistoryURLs(newBack)
|
||||
restoredForwardHistoryStack = Array(Self.sanitizedSessionHistoryURLs(newForward).reversed())
|
||||
restoredHistoryCurrentURL = liveCurrent
|
||||
refreshNavigationAvailability()
|
||||
return
|
||||
}
|
||||
|
||||
guard !restoredForwardHistoryStack.isEmpty else { return }
|
||||
#if DEBUG
|
||||
dlog(
|
||||
"browser.history.restore.forward.clear panel=\(id.uuidString.prefix(5)) " +
|
||||
"current=\(liveCurrentString)"
|
||||
)
|
||||
#endif
|
||||
restoredForwardHistoryStack.removeAll(keepingCapacity: false)
|
||||
refreshNavigationAvailability()
|
||||
}
|
||||
|
||||
func restoreSessionNavigationHistory(
|
||||
|
|
@ -2927,10 +3017,16 @@ final class BrowserPanel: Panel, ObservableObject {
|
|||
webViewObservers.append(titleObserver)
|
||||
|
||||
// Loading state
|
||||
let loadingObserver = webView.observe(\.isLoading, options: [.new]) { [weak self] webView, _ in
|
||||
// Capture the KVO-provided value at observation time rather than reading
|
||||
// webView.isLoading inside the deferred Task. For fast navigations (e.g.
|
||||
// back-forward cache), isLoading can flip true→false before the first Task
|
||||
// runs, causing handleWebViewLoadingChanged(true) to be missed entirely.
|
||||
// That skips favicon/loading-state cleanup and leaves stale icons visible.
|
||||
let loadingObserver = webView.observe(\.isLoading, options: [.new]) { [weak self] webView, change in
|
||||
let newValue = change.newValue ?? webView.isLoading
|
||||
Task { @MainActor in
|
||||
guard let self, self.isCurrentWebView(webView, instanceID: observedWebViewInstanceID) else { return }
|
||||
self.handleWebViewLoadingChanged(webView.isLoading)
|
||||
self.handleWebViewLoadingChanged(newValue)
|
||||
}
|
||||
}
|
||||
webViewObservers.append(loadingObserver)
|
||||
|
|
@ -3209,6 +3305,27 @@ final class BrowserPanel: Panel, ObservableObject {
|
|||
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
|
||||
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }
|
||||
|
||||
// SPAs often inject <link rel="icon"> via JavaScript after the initial
|
||||
// HTML loads. If no link tag was found, wait briefly and retry once to
|
||||
// give client-side scripts time to add the tag.
|
||||
if discoveredURL == nil {
|
||||
try? await Task.sleep(nanoseconds: 600_000_000)
|
||||
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
|
||||
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }
|
||||
if let href = await self.evaluateJavaScriptString(
|
||||
js,
|
||||
in: webView,
|
||||
timeoutNanoseconds: 400_000_000
|
||||
) {
|
||||
let trimmed = href.trimmingCharacters(in: .whitespacesAndNewlines)
|
||||
if !trimmed.isEmpty, let u = URL(string: trimmed) {
|
||||
discoveredURL = u
|
||||
}
|
||||
}
|
||||
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
|
||||
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }
|
||||
}
|
||||
|
||||
let fallbackURL = URL(string: "/favicon.ico", relativeTo: pageURL)
|
||||
let iconURL = discoveredURL ?? fallbackURL
|
||||
guard let iconURL else { return }
|
||||
|
|
@ -3879,20 +3996,29 @@ extension BrowserPanel {
|
|||
func goBack() {
|
||||
guard canGoBack else { return }
|
||||
if usesRestoredSessionHistory {
|
||||
guard let targetURL = restoredBackHistoryStack.popLast() else {
|
||||
realignRestoredSessionHistoryToLiveCurrentIfPossible()
|
||||
|
||||
if (isLiveSessionHistoryAlignedWithRestoredCurrent || !nativeCanGoBack),
|
||||
let targetURL = restoredBackHistoryStack.popLast() {
|
||||
if let current = resolvedCurrentSessionHistoryURL() {
|
||||
restoredForwardHistoryStack.append(current)
|
||||
}
|
||||
restoredHistoryCurrentURL = targetURL
|
||||
refreshNavigationAvailability()
|
||||
navigateWithoutInsecureHTTPPrompt(
|
||||
to: targetURL,
|
||||
recordTypedNavigation: false,
|
||||
preserveRestoredSessionHistory: true
|
||||
)
|
||||
return
|
||||
}
|
||||
if let current = resolvedCurrentSessionHistoryURL() {
|
||||
restoredForwardHistoryStack.append(current)
|
||||
|
||||
if nativeCanGoBack {
|
||||
webView.goBack()
|
||||
return
|
||||
}
|
||||
restoredHistoryCurrentURL = targetURL
|
||||
|
||||
refreshNavigationAvailability()
|
||||
navigateWithoutInsecureHTTPPrompt(
|
||||
to: targetURL,
|
||||
recordTypedNavigation: false,
|
||||
preserveRestoredSessionHistory: true
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -3903,6 +4029,13 @@ extension BrowserPanel {
|
|||
func goForward() {
|
||||
guard canGoForward else { return }
|
||||
if usesRestoredSessionHistory {
|
||||
realignRestoredSessionHistoryToLiveCurrentIfPossible()
|
||||
|
||||
if nativeCanGoForward {
|
||||
webView.goForward()
|
||||
return
|
||||
}
|
||||
|
||||
guard let targetURL = restoredForwardHistoryStack.popLast() else {
|
||||
refreshNavigationAvailability()
|
||||
return
|
||||
|
|
@ -5168,8 +5301,8 @@ extension BrowserPanel {
|
|||
let resolvedCanGoBack: Bool
|
||||
let resolvedCanGoForward: Bool
|
||||
if usesRestoredSessionHistory {
|
||||
resolvedCanGoBack = !restoredBackHistoryStack.isEmpty
|
||||
resolvedCanGoForward = !restoredForwardHistoryStack.isEmpty
|
||||
resolvedCanGoBack = nativeCanGoBack || !restoredBackHistoryStack.isEmpty
|
||||
resolvedCanGoForward = nativeCanGoForward || !restoredForwardHistoryStack.isEmpty
|
||||
} else {
|
||||
resolvedCanGoBack = nativeCanGoBack
|
||||
resolvedCanGoForward = nativeCanGoForward
|
||||
|
|
|
|||
|
|
@ -1515,6 +1515,49 @@ final class BrowserJavaScriptDialogDelegateTests: XCTestCase {
|
|||
|
||||
@MainActor
|
||||
final class BrowserSessionHistoryRestoreTests: XCTestCase {
|
||||
private func writeBrowserFixturePage(
|
||||
at url: URL,
|
||||
title: String,
|
||||
file: StaticString = #filePath,
|
||||
line: UInt = #line
|
||||
) throws {
|
||||
let html = """
|
||||
<html>
|
||||
<head><title>\(title)</title></head>
|
||||
<body>\(title)</body>
|
||||
</html>
|
||||
"""
|
||||
|
||||
do {
|
||||
try html.write(to: url, atomically: true, encoding: .utf8)
|
||||
} catch {
|
||||
XCTFail("Failed to write browser fixture page: \(error)", file: file, line: line)
|
||||
throw error
|
||||
}
|
||||
}
|
||||
|
||||
private func waitForBrowserPanel(
|
||||
_ panel: BrowserPanel,
|
||||
url: URL,
|
||||
timeout: TimeInterval = 5.0,
|
||||
file: StaticString = #filePath,
|
||||
line: UInt = #line
|
||||
) {
|
||||
let deadline = Date().addingTimeInterval(timeout)
|
||||
while Date() < deadline {
|
||||
RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01))
|
||||
if panel.preferredURLStringForOmnibar() == url.absoluteString && !panel.isLoading {
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
XCTFail(
|
||||
"Timed out waiting for browser panel to load \(url.absoluteString). Current=\(panel.preferredURLStringForOmnibar() ?? "nil") loading=\(panel.isLoading)",
|
||||
file: file,
|
||||
line: line
|
||||
)
|
||||
}
|
||||
|
||||
func testSessionNavigationHistorySnapshotUsesRestoredStacks() {
|
||||
let panel = BrowserPanel(workspaceId: UUID())
|
||||
|
||||
|
|
@ -1578,6 +1621,47 @@ final class BrowserSessionHistoryRestoreTests: XCTestCase {
|
|||
XCTAssertTrue(panel.canGoForward)
|
||||
}
|
||||
|
||||
func testGoBackPrefersLiveWKWebViewHistoryBeforeRestoredFallback() throws {
|
||||
let tempDir = FileManager.default.temporaryDirectory
|
||||
.appendingPathComponent("cmux-browser-history-\(UUID().uuidString)", isDirectory: true)
|
||||
try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true)
|
||||
defer { try? FileManager.default.removeItem(at: tempDir) }
|
||||
|
||||
let pageA = tempDir.appendingPathComponent("a.html")
|
||||
let pageB = tempDir.appendingPathComponent("b.html")
|
||||
let pageC = tempDir.appendingPathComponent("c.html")
|
||||
try writeBrowserFixturePage(at: pageA, title: "A")
|
||||
try writeBrowserFixturePage(at: pageB, title: "B")
|
||||
try writeBrowserFixturePage(at: pageC, title: "C")
|
||||
|
||||
let panel = BrowserPanel(
|
||||
workspaceId: UUID(),
|
||||
initialURL: pageB
|
||||
)
|
||||
waitForBrowserPanel(panel, url: pageB)
|
||||
|
||||
panel.restoreSessionNavigationHistory(
|
||||
backHistoryURLStrings: [pageA.absoluteString],
|
||||
forwardHistoryURLStrings: [],
|
||||
currentURLString: pageB.absoluteString
|
||||
)
|
||||
|
||||
_ = browserLoadRequest(URLRequest(url: pageC), in: panel.webView)
|
||||
waitForBrowserPanel(panel, url: pageC)
|
||||
|
||||
let snapshot = panel.sessionNavigationHistorySnapshot()
|
||||
XCTAssertEqual(
|
||||
snapshot.backHistoryURLStrings,
|
||||
[pageA.absoluteString, pageB.absoluteString]
|
||||
)
|
||||
|
||||
panel.goBack()
|
||||
waitForBrowserPanel(panel, url: pageB)
|
||||
|
||||
panel.goBack()
|
||||
waitForBrowserPanel(panel, url: pageA)
|
||||
}
|
||||
|
||||
func testWebViewReplacementAfterProcessTerminationUpdatesInstanceIdentity() {
|
||||
let panel = BrowserPanel(
|
||||
workspaceId: UUID(),
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue