fix page position on navigation #2330

brindy · 2024-01-11T17:11:35Z

Task/Issue URL: https://app.asana.com/0/715106103902962/1205688349294686/f
Tech Design URL:
CC:

Description:
Fixes the following bugs:

page position would be forgotten on navigation
scrolling would stop when user scrolls the webview and the bars are being hidden

Steps to test this PR:

On both iPhone and iPad, ideally on a device:

Browse the web and use backwards and forwards navigation. The page position should not change on navigation.
Check different orientations
Check keyboard adjusts the webview correctly as needed (when entering fields on the page)
Check the browsing menu appears in the expected location

On iPhone specifically (or iPad when in small mode on split view):

Check browsing with top and bottom address bar positions (bottom bar position not available on iPad unless in “small” mode / split screen)
Check scrolling hides and shows the bar as previous, but does not stop the page scrolling
Quick swipe should hide show bars as previous
When at bottom of the screen over scrolling should show bars as previous

… is now a different size

brindy · 2024-01-12T14:05:20Z

DuckDuckGo/BarsAnimator.swift

-
-        var offset = scrollView.contentOffset
-        offset.y = transitionStartPosY
-        scrollView.contentOffset = offset


adjusting the content offset while it was scrolling is what causes the page to stop scrolling while the bars are being hidden, which is not what other browsers do and at least one person internally has complained about

Do you know why we were doing this?

Not really, but it's been flagged as a bug since and does seem inconsistent with other browsers and personally I think it is a bit annoying :)

brindy · 2024-01-12T14:05:38Z

DuckDuckGo/BarsAnimator.swift

-        return min(normalizedDistance / barsHeight, 1.0)
+        // We used to fix the scroll position in place as the transition happened
+        //  but now the bars disappear too. This adjusts for that.
+        let transitionSpeed = 0.5


This is related to the content offset code removal above.

brindy · 2024-01-12T14:05:55Z

DuckDuckGo/BrowserChromeManager.swift

@@ -38,7 +38,6 @@ protocol BrowserChromeDelegate: AnyObject {
 class BrowserChromeManager: NSObject, UIScrollViewDelegate {

    struct Constants {
-        static let dragThreshold: CGFloat = 30


brindy · 2024-01-12T14:06:38Z

DuckDuckGo/BrowsingMenu/BrowsingMenuAnimator.swift

@@ -61,7 +61,7 @@ final class BrowsingMenuAnimator: NSObject, UIViewControllerAnimatedTransitionin
            transitionContext.containerView.addSubview(fromSnapshot)
        }

-        toViewController.bindConstraints(to: fromViewController.currentTab?.webView)
+        toViewController.bindConstraints(to: fromViewController.viewCoordinator.contentContainer)


the web view can now be as big as the app so is the wrong thing to base the browsing menu on

brindy · 2024-01-12T14:15:13Z

DuckDuckGo/BrowsingMenu/BrowsingMenuViewController.swift

-        self.webView = webView
-        self.webViewFrameObserver = webView?.observe(\.frame, options: [.initial]) { [weak self] webView, _ in
-            self?.recalculateMenuConstraints(with: webView)
+    private weak var guideView: UIView?


since it's no longer a web view now, I've renamed it to match its purpose

brindy · 2024-01-12T14:16:05Z

DuckDuckGo/MainViewController.swift

@@ -137,6 +137,8 @@ class MainViewController: UIViewController {

    // Skip SERP flow (focusing on autocomplete logic) and prepare for new navigation when selecting search bar
    private var skipSERPFlow = true
+
+    private var keyboardHeight: CGFloat = 0.0


Set when the keyboard frame changes so that it can be used when refreshing the content insets for the web view

brindy · 2024-01-12T14:16:32Z

DuckDuckGo/MainViewController.swift

@@ -872,18 +876,29 @@ class MainViewController: UIViewController {
        }
    }

-    private func addToView(tab: TabViewController) {
+    private func addToWebViewContainer(tab: TabViewController) {


renamed to reflect purpose instead of unclear overloaded names

brindy · 2024-01-12T14:16:36Z

DuckDuckGo/MainViewController.swift

        tab.progressWorker.progressBar = viewCoordinator.progress
        chromeManager.attach(to: tab.webView.scrollView)
        tab.chromeDelegate = self
    }

-    private func addToView(controller: UIViewController) {
+    private func addToContentContainer(controller: UIViewController) {


renamed to reflect purpose

brindy · 2024-01-12T14:37:48Z

DuckDuckGo/MainViewController.swift

        removeHomeScreen()
        updateFindInPage()
        currentTab?.progressWorker.progressBar = nil
        currentTab?.chromeDelegate = nil
-        addToView(controller: tab)
+        currentTab?.webView.scrollView.contentInsetAdjustmentBehavior = .never


iOS does weird stuff if this not disabled, once you start messing with the content insets

brindy · 2024-01-12T14:38:26Z

DuckDuckGo/TabViewController.swift

+                              height: webView.frame.size.height - webView.scrollView.contentInset.top - webView.scrollView.contentInset.bottom)
+
+            UIGraphicsBeginImageContextWithOptions(size, false, UIScreen.main.scale)
+            UIGraphicsGetCurrentContext()?.translateBy(x: 0, y: -webView.scrollView.contentInset.top)


we're only interested in the bit of the screen currently visible - without this we end up with potentially blank space

brindy · 2024-01-12T14:39:31Z

Just did a bit of a self review and pushed a couple of commits. I'm definitely done now @THISISDINOSAUR when you're ready. Thanks!

THISISDINOSAUR

LGTM!
The change with scrolling when the URL bar is at the bottom is way better.

I haven't been able to get it to forget the scroll position, but that bug was always really hard to replicate. It at least doesn't seem worse 😅

brindy · 2024-01-15T16:40:14Z

Thank you!

brindy added 8 commits January 9, 2024 14:20

rough implementation

9dade03

experiment with not using zoom to adjust bars

f2a3c8c

restore scrolling hide / show bars

612a0fc

use content container as guide for the menu instead of webview, which…

b5f90ee

… is now a different size

Merge branch 'main' into brindy/fix-page-position-on-navigation

65217d8

Merge branch 'main' into brindy/fix-page-position-on-navigation

ec39ef4

use the right size for preview image when transitioning back to web view

7156740

correct grab tab preview

82a6db2

duckduckgo deleted a comment from github-actions bot Jan 11, 2024

brindy added 6 commits January 12, 2024 10:14

remove unused constant

f88f0f3

refresh the insets when something changes in the ui

41f3197

fix scrolling problem

0bf4dd5

set inset correctly for bar positions

c692cfe

build keyboard frame into content inset calculation

657b50a

use a dedicated container view in the coordinator

9e68c9e

brindy marked this pull request as ready for review January 12, 2024 13:21

brindy requested review from a team, amddg44 and THISISDINOSAUR and removed request for a team and amddg44 January 12, 2024 13:35

brindy commented Jan 12, 2024

View reviewed changes

use guideview not parent

6e5d978

brindy commented Jan 12, 2024

View reviewed changes

brindy added 2 commits January 12, 2024 14:17

use nonoptional var

4c53235

calculate preview size without needing main view controller

115b040

brindy commented Jan 12, 2024

View reviewed changes

fix bug when restarting where view would be too big

c10dd4e

THISISDINOSAUR approved these changes Jan 15, 2024

View reviewed changes

brindy merged commit 8b94de5 into main Jan 15, 2024
11 checks passed

brindy deleted the brindy/fix-page-position-on-navigation branch January 15, 2024 16:40

samsymons mentioned this pull request Jan 15, 2024

Bottom Bar Bug – Hiding but keeping blank area after scroll #2159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix page position on navigation #2330

fix page position on navigation #2330

brindy commented Jan 11, 2024 •

edited

Loading

brindy Jan 12, 2024

THISISDINOSAUR Jan 15, 2024

brindy Jan 15, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy Jan 12, 2024

brindy commented Jan 12, 2024

THISISDINOSAUR left a comment

brindy commented Jan 15, 2024

fix page position on navigation #2330

fix page position on navigation #2330

Conversation

brindy commented Jan 11, 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

brindy commented Jan 12, 2024

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

brindy commented Jan 15, 2024

brindy commented Jan 11, 2024 •

edited

Loading