Fixed address bar cleared/losing focus on Navigation #2157

mallexxx · 2024-02-05T16:23:50Z

Task/Issue URL: https://app.asana.com/0/1177771139624306/1206506638670996/f

Description:

Fixed user input overwritten in the Address Bar on Web View navigation (preserving input until url changes when the address bar is not the first responder)
Fixed Address Bar losing focus on non-user-initiated navigations
Fixed minor nasty issues like:
empty address bar on Browser Launch + State Restoration
focused address bar on State Restoration
delayed Address Bar update, focus state change on tab switching
Slightly updated the Address Bar text update logics + spoofing prevention
Updating the address bar text on Navigation Did Commit event instead of Loading getting 100% - should be more accurate (CC @not-a-rootkit- ✓ Address bar behavior changed after url confirmation in 1.66.0)
Fixed the leaking Cancellable - created on each navigation in the loading progress subscription

Steps to test this PR:

Launch browser with an empty tab open: start typing quickly before the browser window appears; Validate when the new tab is displayed, all the entered text is there and not being selected or reset
Launch browser with a restored tab: validate url is displayed right away; Quickly activate the address bar and start typing, validate the input is not reset during redirections
[+test] Switch between loaded tabs and home tab/settings/bookmarks, validate the Address Bar gets activated on the New Tab; Validate privacy entry button/icon is correct
Enter something, switch to another tab, enter something, return back, validate the input is preserved, return to tab 2, validate its input is preserved
Enter something, open some bookmark, history item, html file by dragging it to the icon – validate the address bar is deactivated and reset
Validate some spoofing scenario from https://app.asana.com/0/1199230911884351/1205844442726343/f
Open new tab - validate address bar is activated; Navigate somewhere, validate address bar is deactivated; go back to the home page - validate address bar is activated; navigate forward: deactivated; make sure privacy entry point icon doesn‘t blink
Navigate somewhere, then somewhere else; activate the Address Bar, go back, validate the address bar is deactivated
Open new tab; Open another new tab - address bar should remain active; close the 2nd tab - address bar should remain active
Navigate to settings, bookmarks, onboarding - validate address bar is not active
Session restore to a website; to new tab; to settings; to bookmarks etc - validate address bar is activated/deactivated accordingly
Open 2 tabs, navigate both somewhere, activate the address bar, switch to another tab - validate the address bar is deactivated
Open SERP, enter something in the address bar, click the Search button in SERP (submitting the same query) - validate the address bar text is reset

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…cleared-on-nav

tomasstrba

@mallexxx , I started with testing and found this issue where activating another new tab doesn't hide suggestions
https://github.com/duckduckgo/macos-browser/assets/57389842/c2b1134a-57ba-480a-b176-b0084a09b026

mallexxx · 2024-02-07T16:34:33Z

good catch, @tomasstrba! fixed

tomasstrba · 2024-02-11T18:21:39Z

@mallexxx, switching between homepage and the regular tab makes this weird visual glitch in the address bar. You'll better see it if you download the video and go frame by frame

glitch.mov

It is caused by this frame:

…cleared-on-nav

github-actions · 2024-02-19T14:20:40Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against a968834

…cleared-on-nav

mallexxx · 2024-02-22T12:54:57Z

Configuration/UITests/UITests.xcconfig

-LD_RUNPATH_SEARCH_PATHS = $(inherited) @executable_path/../Frameworks @loader_path/../Frameworks
+PROVISIONING_PROFILE_SPECIFIER[config=CI][sdk=macosx*] =
+SWIFT_EMIT_LOC_STRINGS = NO
+TEST_TARGET_NAME = DuckDuckGo Privacy Browser


not really related change but this made UI test at least launchable

mallexxx · 2024-02-22T12:55:30Z

DuckDuckGo/Application/AppDelegate.swift

@@ -240,7 +240,9 @@ final class AppDelegate: NSObject, NSApplicationDelegate, FileDownloadManagerDel

        startupSync()

-        stateRestorationManager.applicationDidFinishLaunching()
+        if [.normal, .uiTests].contains(NSApp.runType) {
+            stateRestorationManager.applicationDidFinishLaunching()


disable state restoration for integration tests

mallexxx · 2024-02-22T12:56:29Z

DuckDuckGo/MainWindow/MainView.swift

@@ -61,7 +61,6 @@ final class MainView: NSView {
        bookmarksBarHeightConstraint = bookmarksBarContainerView.heightAnchor.constraint(equalToConstant: 34)

        navigationBarTopConstraint = navigationBarContainerView.topAnchor.constraint(equalTo: topAnchor, constant: 38)
-        addressBarHeightConstraint = navigationBarContainerView.heightAnchor.constraint(equalToConstant: 42)


moved tnis into the Navigation Bar Controller in sake of encapsulation and fluid address bar animation

mallexxx · 2024-02-22T12:58:13Z

DuckDuckGo/MainWindow/MainWindowController.swift

@@ -314,10 +314,7 @@ fileprivate extension MainMenu {
 fileprivate extension NavigationBarViewController {

    var controlsForUserPrevention: [NSControl?] {
-        return [goBackButton,


this was enabling back/forward/refresh buttons after onboarding; they are disabled by default as there‘s no nav history yet

mallexxx · 2024-02-22T13:02:48Z

DuckDuckGo/NavigationBar/View/AddressBarViewController.swift

@@ -319,6 +327,8 @@ final class AddressBarViewController: NSViewController {
        activeBackgroundView.layer?.borderColor = NSColor.controlAccentColor.withAlphaComponent(0.8).cgColor
        activeOuterBorderView.layer?.backgroundColor = accentColor.withAlphaComponent(0.2).cgColor
        activeBackgroundView.layer?.borderColor = accentColor.withAlphaComponent(0.8).cgColor
+
+        addressBarTextField.placeholderString = tabViewModel?.tab.content == .newtab ? UserText.addressBarPlaceholder : ""


prevent placeholder blinking when going forward from newtab to a url

mallexxx · 2024-02-22T13:03:27Z

DuckDuckGo/Tab/Model/Tab.swift

@@ -680,17 +680,16 @@ protocol NewWindowPolicyDecisionMaker {
            }
            return
        }
-        var title = webView.title?.trimmingWhitespace()
-        if title?.isEmpty ?? true {


we‘ve got this in TabViewModel

mallexxx · 2024-02-22T13:03:39Z

DuckDuckGo/Tab/Model/Tab.swift

        if title != self.title {
            self.title = title
        }

        if let wkBackForwardListItem = webView.backForwardList.currentItem,
-           content.urlForWebView == wkBackForwardListItem.url {
+           content.urlForWebView == wkBackForwardListItem.url,
+           !webView.isLoading,


prevent updating wrong history item while loading

mallexxx · 2024-02-22T13:04:14Z

DuckDuckGo/Tab/Model/Tab.swift

+            guard let self,
+                  case .url(let url, credential: let credential, .webViewUpdated) = self.content,
+                  url == navigation.url else { return }
+            self.content = .url(url, credential: credential, source: .historyEntry)


change content type from .webViewUpdated to .historyEntry so the address bar is deactivated on back/forward/refresh

mallexxx · 2024-02-22T13:04:38Z

DuckDuckGo/Tab/Model/Tab.swift

@@ -1285,6 +1308,7 @@ extension Tab/*: NavigationResponder*/ { // to be moved to Tab+Navigation.swift
    func didStart(_ navigation: Navigation) {
        webViewDidStartNavigationPublisher.send()
        delegate?.tabDidStartNavigation(self)
+        permissions.tabDidStartNavigation()


fix encapsulation

mallexxx · 2024-02-22T13:05:18Z

DuckDuckGo/Tab/View/BrowserTabView.swift

@@ -20,6 +20,19 @@ import AppKit

 final class BrowserTabView: ColorView {

+    // Returns correct subview for the rendering of snapshots
+    func findContentSubview(containsHostingView: Bool) -> NSView? {


this was raising file too long linter issue, moved to the custom view

mallexxx · 2024-02-22T13:06:06Z

DuckDuckGo/Tab/View/BrowserTabViewController.swift

+            getView = { [weak self] in self?.transientTabContentViewController?.view }
+        case .url, .subscription:
+            getView = { [weak self] in self?.webView }
+        case .settings:
+            getView = { [weak self] in self?.preferencesViewController?.view }
+        case .bookmarks:
+            getView = { [weak self] in self?.bookmarksViewController?.view }
+        case .dataBrokerProtection:
+            getView = { [weak self] in self?.dataBrokerProtectionHomeViewController?.view }


if view is not yet in hierarchy - add it afterwards

mallexxx · 2024-02-22T13:07:19Z

DuckDuckGo/Tab/ViewModel/TabViewModel.swift

+                updateAddressBarStrings()
+                if case .didCommit = event {
+                    updateCanBeBookmarked()
+                    updateFavicon()
                }


address bar should update instantly for some kind of contents except for web view updated; but the favicon and button shouldn‘t blink

mallexxx · 2024-02-22T13:07:53Z

DuckDuckGo/Tab/ViewModel/TabViewModel.swift

+                self?.updateTitle()
+                self?.updateFavicon()


only update on actual error state change to avoid blinking

tomasstrba

Address bar activation/deactivation functionality works great now! 👍 I don't see any animation glitch. There are just few details I observed:

There is a separator below the navigation bar on the new tab page, which shouldn't be there

I am not sure if this PR influenced it, but switching tabs is slower. Try switching between new tab page and random regular tabs.
Also, there is a significant lag until the webview is presented when SERP is triggered from the address bar.

DuckDuckGo/NavigationBar/View/AddressBarTextField.swift

tomasstrba · 2024-02-23T08:32:06Z

IntegrationTests/Tab/AddressBarTests.swift

…cleared-on-nav

mallexxx · 2024-02-26T09:34:52Z

Thanks, @tomasstrba, I‘ve fixed the divider issue;
Frankly I don‘t see any glitches on tab switching or SERP loading, I‘ve checked: web view is being added on NavigationDidStart, tab switching seems working fast..
I did see some slowdown on switching to the New Tab page some time ago and it seemed like the heavy loaded SwiftUI issue - I hope that should be fixed when we switch to a native home page

tomasstrba

LGTM! 👍 Good job! 👏

…cleared-on-nav

Fixed address bar cleared/losing focus on Navigation

03f9bb3

mallexxx requested a review from tomasstrba February 5, 2024 16:23

mallexxx assigned tomasstrba Feb 7, 2024

mallexxx added 2 commits February 7, 2024 12:29

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

375cbb1

…cleared-on-nav

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

c5cdd45

…cleared-on-nav

tomasstrba reviewed Feb 7, 2024

View reviewed changes

fixed suggestion window not disappearing on tab switch

7b699bf

tomasstrba assigned mallexxx and unassigned tomasstrba Feb 11, 2024

mallexxx added 2 commits February 19, 2024 14:29

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

3aa45ff

…cleared-on-nav

refactor first responder setting and fix address bar blinking issues

8f37cd8

mallexxx assigned tomasstrba and unassigned mallexxx Feb 19, 2024

mallexxx added 6 commits February 19, 2024 21:07

fix tests

7ebe607

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

543f5ba

…cleared-on-nav

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

535f92e

…cleared-on-nav

fix issues, add tests

41ae4cf

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

eeb4e1c

…cleared-on-nav

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

d0c5643

…cleared-on-nav

mallexxx commented Feb 22, 2024

View reviewed changes

fix comment; fixing tests

808c5bd

tomasstrba reviewed Feb 23, 2024

View reviewed changes

tomasstrba assigned mallexxx Feb 23, 2024

mallexxx added 2 commits February 26, 2024 15:14

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

3b634cb

…cleared-on-nav

fix divider shown on the Home Page

5684bb5

tomasstrba approved these changes Feb 26, 2024

View reviewed changes

mallexxx added 2 commits February 27, 2024 12:59

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

52d6f16

…cleared-on-nav

Merge remote-tracking branch 'origin/main' into alex/fix-address-bar-…

a968834

…cleared-on-nav

mallexxx merged commit 99f8049 into main Feb 27, 2024
17 checks passed

mallexxx deleted the alex/fix-address-bar-cleared-on-nav branch February 27, 2024 11:15

ayoy mentioned this pull request Mar 8, 2024

Limit force reloading in Tab.reload() #2361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed address bar cleared/losing focus on Navigation #2157

Fixed address bar cleared/losing focus on Navigation #2157

mallexxx commented Feb 5, 2024 •

edited

Loading

tomasstrba left a comment

mallexxx commented Feb 7, 2024

tomasstrba commented Feb 11, 2024

github-actions bot commented Feb 19, 2024 •

edited

Loading

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

mallexxx Feb 22, 2024

tomasstrba left a comment

tomasstrba Feb 23, 2024

mallexxx commented Feb 26, 2024

tomasstrba left a comment

Fixed address bar cleared/losing focus on Navigation #2157

Fixed address bar cleared/losing focus on Navigation #2157

Conversation

mallexxx commented Feb 5, 2024 • edited Loading

Internal references:

tomasstrba left a comment

Choose a reason for hiding this comment

mallexxx commented Feb 7, 2024

tomasstrba commented Feb 11, 2024

github-actions bot commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasstrba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mallexxx commented Feb 26, 2024

tomasstrba left a comment

Choose a reason for hiding this comment

mallexxx commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading