-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix page position on navigation #2330
Changes from all commits
9dade03
f2a3c8c
612a0fc
b5f90ee
65217d8
ec39ef4
7156740
82a6db2
f88f0f3
41f3197
0bf4dd5
c692cfe
657b50a
9e68c9e
6e5d978
4c53235
115b040
c10dd4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,10 +84,6 @@ class BarsAnimator { | |
let ratio = calculateTransitionRatio(for: scrollView.contentOffset.y) | ||
delegate?.setBarsVisibility(1.0 - ratio, animated: false) | ||
transitionProgress = ratio | ||
|
||
var offset = scrollView.contentOffset | ||
offset.y = transitionStartPosY | ||
scrollView.contentOffset = offset | ||
} | ||
|
||
private func transitioningAndScrolling(in scrollView: UIScrollView) { | ||
|
@@ -103,10 +99,6 @@ class BarsAnimator { | |
|
||
delegate?.setBarsVisibility(1.0 - ratio, animated: false) | ||
transitionProgress = ratio | ||
|
||
var offset = scrollView.contentOffset | ||
offset.y = transitionStartPosY | ||
scrollView.contentOffset = offset | ||
} | ||
|
||
private func hiddenAndScrolling(in scrollView: UIScrollView) { | ||
|
@@ -139,7 +131,12 @@ class BarsAnimator { | |
let cumulativeDistance = (barsHeight * transitionProgress) + distance | ||
let normalizedDistance = max(cumulativeDistance, 0) | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to the content offset code removal above. |
||
|
||
let ratio = min(normalizedDistance / barsHeight * transitionSpeed, 1.0) | ||
return ratio | ||
} | ||
|
||
func didFinishScrolling(in scrollView: UIScrollView, velocity: CGFloat) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,6 @@ protocol BrowserChromeDelegate: AnyObject { | |
class BrowserChromeManager: NSObject, UIScrollViewDelegate { | ||
|
||
struct Constants { | ||
static let dragThreshold: CGFloat = 30 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unused |
||
static let zoomThreshold: CGFloat = 0.1 | ||
|
||
static let contentSizeKVOKey = "contentSize" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the web view can now be as big as the app so is the wrong thing to base the browsing menu on |
||
toViewController.view.layoutIfNeeded() | ||
|
||
let snapshot = toViewController.menuView.snapshotView(afterScreenUpdates: true) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,7 +157,7 @@ final class BrowsingMenuViewController: UIViewController { | |
|
||
recalculatePreferredWidthConstraint() | ||
recalculateHeightConstraints() | ||
webView.map(recalculateMenuConstraints(with:)) | ||
guideView.map(recalculateMenuConstraints(with:)) | ||
|
||
if tableView.bounds.height < tableView.contentSize.height + tableView.contentInset.top + tableView.contentInset.bottom { | ||
tableView.isScrollEnabled = true | ||
|
@@ -166,12 +166,12 @@ final class BrowsingMenuViewController: UIViewController { | |
} | ||
} | ||
|
||
private weak var webView: UIView? | ||
private var webViewFrameObserver: NSKeyValueObservation? | ||
func bindConstraints(to webView: UIView?) { | ||
self.webView = webView | ||
self.webViewFrameObserver = webView?.observe(\.frame, options: [.initial]) { [weak self] webView, _ in | ||
self?.recalculateMenuConstraints(with: webView) | ||
private weak var guideView: UIView? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since it's no longer a web view now, I've renamed it to match its purpose |
||
private var guideViewFrameObserver: NSKeyValueObservation? | ||
func bindConstraints(to guideView: UIView?) { | ||
self.guideView = guideView | ||
self.guideViewFrameObserver = guideView?.observe(\.frame, options: [.initial]) { [weak self] guideView, _ in | ||
self?.recalculateMenuConstraints(with: guideView) | ||
} | ||
} | ||
|
||
|
@@ -205,9 +205,9 @@ final class BrowsingMenuViewController: UIViewController { | |
} | ||
} | ||
|
||
private func recalculateMenuConstraints(with webView: UIView) { | ||
guard let frame = webView.superview?.convert(webView.frame, to: webView.window), | ||
let windowBounds = webView.window?.bounds | ||
private func recalculateMenuConstraints(with guideView: UIView) { | ||
guard let frame = guideView.superview?.convert(guideView.frame, to: guideView.window), | ||
let windowBounds = guideView.window?.bounds | ||
else { return } | ||
|
||
let isIPad = AppWidthObserver.shared.isLargeWidth | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set when the keyboard frame changes so that it can be used when refreshing the content insets for the web view |
||
|
||
required init?(coder: NSCoder) { | ||
fatalError("Use init?(code:") | ||
|
@@ -430,6 +432,7 @@ class MainViewController: UIViewController { | |
@objc func onAddressBarPositionChanged() { | ||
viewCoordinator.moveAddressBarToPosition(appSettings.currentAddressBarPosition) | ||
refreshViewsBasedOnAddressBarPosition(appSettings.currentAddressBarPosition) | ||
refreshWebViewContentInsets() | ||
} | ||
|
||
func refreshViewsBasedOnAddressBarPosition(_ position: AddressBarPosition) { | ||
|
@@ -475,7 +478,8 @@ class MainViewController: UIViewController { | |
height = intersection.height | ||
|
||
findInPageBottomLayoutConstraint.constant = height | ||
currentTab?.webView.scrollView.contentInset = UIEdgeInsets(top: 0, left: 0, bottom: height, right: 0) | ||
keyboardHeight = height | ||
refreshWebViewContentInsets() | ||
|
||
if let suggestionsTray = suggestionTrayController { | ||
let suggestionsFrameInView = suggestionsTray.view.convert(suggestionsTray.contentFrame, to: view) | ||
|
@@ -640,7 +644,7 @@ class MainViewController: UIViewController { | |
guard let tab = tabManager.current(createIfNeeded: true) else { | ||
fatalError("Unable to create tab") | ||
} | ||
addToView(tab: tab) | ||
addToWebViewContainer(tab: tab) | ||
refreshControls() | ||
} else { | ||
attachHomeScreen() | ||
|
@@ -700,7 +704,7 @@ class MainViewController: UIViewController { | |
controller.chromeDelegate = self | ||
controller.delegate = self | ||
|
||
addToView(controller: controller) | ||
addToContentContainer(controller: controller) | ||
|
||
refreshControls() | ||
syncService.scheduler.requestSyncImmediately() | ||
|
@@ -849,7 +853,7 @@ class MainViewController: UIViewController { | |
private func addTab(url: URL?, inheritedAttribution: AdClickAttributionLogic.State?) { | ||
let tab = tabManager.add(url: url, inheritedAttribution: inheritedAttribution) | ||
dismissOmniBar() | ||
addToView(tab: tab) | ||
addToWebViewContainer(tab: tab) | ||
} | ||
|
||
func select(tabAt index: Int) { | ||
|
@@ -863,7 +867,7 @@ class MainViewController: UIViewController { | |
if tab.link == nil { | ||
attachHomeScreen() | ||
} else { | ||
addToView(tab: tab) | ||
addToWebViewContainer(tab: tab) | ||
refreshControls() | ||
} | ||
tabsBarController?.refresh(tabsModel: tabManager.model, scrollToSelected: true) | ||
|
@@ -872,18 +876,29 @@ class MainViewController: UIViewController { | |
} | ||
} | ||
|
||
private func addToView(tab: TabViewController) { | ||
private func addToWebViewContainer(tab: TabViewController) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed to reflect purpose instead of unclear overloaded names |
||
removeHomeScreen() | ||
updateFindInPage() | ||
currentTab?.progressWorker.progressBar = nil | ||
currentTab?.chromeDelegate = nil | ||
addToView(controller: tab) | ||
currentTab?.webView.scrollView.contentInsetAdjustmentBehavior = .never | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iOS does weird stuff if this not disabled, once you start messing with the content insets |
||
|
||
addChild(tab) | ||
viewCoordinator.webViewContainer.subviews.forEach { $0.removeFromSuperview() } | ||
viewCoordinator.webViewContainer.addSubview(tab.view) | ||
tab.view.frame = self.viewCoordinator.webViewContainer.bounds | ||
tab.didMove(toParent: self) | ||
|
||
viewCoordinator.logoContainer.isHidden = true | ||
viewCoordinator.contentContainer.isHidden = true | ||
|
||
tab.progressWorker.progressBar = viewCoordinator.progress | ||
chromeManager.attach(to: tab.webView.scrollView) | ||
tab.chromeDelegate = self | ||
} | ||
|
||
private func addToView(controller: UIViewController) { | ||
private func addToContentContainer(controller: UIViewController) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed to reflect purpose |
||
viewCoordinator.contentContainer.isHidden = false | ||
addChild(controller) | ||
viewCoordinator.contentContainer.subviews.forEach { $0.removeFromSuperview() } | ||
viewCoordinator.contentContainer.addSubview(controller.view) | ||
|
@@ -1313,28 +1328,53 @@ extension MainViewController: BrowserChromeDelegate { | |
let updateBlock = { | ||
self.updateToolbarConstant(percent) | ||
self.updateNavBarConstant(percent) | ||
|
||
self.view.layoutIfNeeded() | ||
|
||
self.viewCoordinator.omniBar.alpha = percent | ||
self.viewCoordinator.navigationBarContainer.alpha = percent | ||
self.viewCoordinator.tabBarContainer.alpha = percent | ||
self.viewCoordinator.toolbar.alpha = percent | ||
} | ||
|
||
if animated { | ||
UIView.animate(withDuration: ChromeAnimationConstants.duration, animations: updateBlock) | ||
UIView.animate(withDuration: ChromeAnimationConstants.duration, animations: updateBlock) { _ in | ||
self.refreshWebViewContentInsets() | ||
} | ||
} else { | ||
updateBlock() | ||
self.refreshWebViewContentInsets() | ||
} | ||
} | ||
|
||
func refreshWebViewContentInsets() { | ||
guard let webView = currentTab?.webView else { return } | ||
|
||
let top = viewCoordinator.statusBackground.frame.height | ||
let bottom: CGFloat | ||
if isToolbarHidden { | ||
bottom = 0 | ||
} else if appSettings.currentAddressBarPosition.isBottom { | ||
bottom = viewCoordinator.toolbar.frame.height | ||
+ viewCoordinator.navigationBarContainer.frame.height | ||
+ view.safeAreaInsets.bottom + additionalSafeAreaInsets.bottom | ||
+ keyboardHeight | ||
} else { | ||
bottom = viewCoordinator.toolbar.frame.height | ||
+ view.safeAreaInsets.bottom + additionalSafeAreaInsets.bottom | ||
+ keyboardHeight | ||
} | ||
|
||
webView.scrollView.contentInset = .init(top: top, left: 0, bottom: bottom, right: 0) | ||
} | ||
|
||
func setNavigationBarHidden(_ hidden: Bool) { | ||
if hidden { hideKeyboard() } | ||
|
||
updateNavBarConstant(hidden ? 0 : 1.0) | ||
viewCoordinator.omniBar.alpha = hidden ? 0 : 1 | ||
viewCoordinator.tabBarContainer.alpha = hidden ? 0 : 1 | ||
viewCoordinator.statusBackground.alpha = hidden ? 0 : 1 | ||
|
||
} | ||
|
||
var canHideBars: Bool { | ||
|
@@ -1692,7 +1732,7 @@ extension MainViewController: TabDelegate { | |
guard self.tabManager.model.tabs.contains(newTab.tabModel) else { return } | ||
|
||
self.dismissOmniBar() | ||
self.addToView(tab: newTab) | ||
self.addToWebViewContainer(tab: newTab) | ||
self.refreshOmniBar() | ||
} | ||
|
||
|
@@ -1796,6 +1836,7 @@ extension MainViewController: TabDelegate { | |
|
||
func showBars() { | ||
chromeManager.reset() | ||
refreshWebViewContentInsets() | ||
} | ||
|
||
func tabDidRequestFindInPage(tab: TabViewController) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1147,7 +1147,12 @@ extension TabViewController: WKNavigationDelegate { | |
DispatchQueue.main.async { [weak self] in | ||
guard let webView = self?.webView, | ||
webView.bounds.height > 0 && webView.bounds.width > 0 else { completion(nil); return } | ||
UIGraphicsBeginImageContextWithOptions(webView.bounds.size, false, UIScreen.main.scale) | ||
|
||
let size = CGSize(width: webView.frame.size.width, | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're only interested in the bit of the screen currently visible - without this we end up with potentially blank space |
||
webView.drawHierarchy(in: webView.bounds, afterScreenUpdates: true) | ||
if let jsAlertController = self?.jsAlertController { | ||
jsAlertController.view.drawHierarchy(in: jsAlertController.view.bounds, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why we were doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)