Fix bug where Back navigation doesn't work properly with bangs #1503
Conversation
mallexxx
left a comment
mallexxx
left a comment
There was a problem hiding this comment.
I‘ve added several code review comments inline, but the fix is generally not something that‘s really needed here:
The main issue is that the redirect item (duckduckgo.com) shouldn‘t appear in navigation history at all - redirects should be automatically suppressed by the WebKit.
Although we may be breaking it sometimes by breaking a redirect sequence with a developer-initiated redirect. This can be debugged by activating Debug->Logging->Navigation log and inspecting redirect sequences. The log shows that the redirect sequence break is caused by a redirect in SerpHeadersNavigationResponder. It should use a helper redirectInvalidatingBackItemIfNeeded call which is wiping the ongoing navigation url from the navigation history effectively fixing the issue.
I‘ve opened #1510 with a working fix and suggest closing this one.
|
|
||
| // MARK: - Search with bang | ||
|
|
||
| private static let searchWithBangPattern = "^.+\\?q=%21[a-zA-Z]+(?:%20[a-zA-Z]+)*$" |
There was a problem hiding this comment.
this needs some comment describing what the regex does as it‘s non-trivial
| private static let searchWithBangPattern = "^.+\\?q=%21[a-zA-Z]+(?:%20[a-zA-Z]+)*$" | ||
|
|
||
| private static var compiledSearchWithBangRegex: NSRegularExpression? = { | ||
| if let newRegex = try? NSRegularExpression(pattern: searchWithBangPattern, options: .caseInsensitive) { |
There was a problem hiding this comment.
Please check BSK.Common.StringExtension -> RegEx for regex instantiation helper (to be updated with compile-time checked macro with xc15)
| } | ||
|
|
||
| let canGoBack = webView.canGoBack || self.error != nil | ||
| let isJustOneRidirectGoingBack = webView.backForwardList.backList.count == 1 && webView.backForwardList.backItem?.url.absoluteString.contains("%21") ?? false |
There was a problem hiding this comment.
this seems error-prone since it‘s checking for "%21" for every url
|
|
||
| let canGoBack = webView.canGoBack || self.error != nil | ||
| let isJustOneRidirectGoingBack = webView.backForwardList.backList.count == 1 && webView.backForwardList.backItem?.url.absoluteString.contains("%21") ?? false | ||
| let canGoBack = (webView.canGoBack && !isJustOneRidirectGoingBack) || self.error != nil |
There was a problem hiding this comment.
the meaning of the check is non-straightforward and would be good if it had some comment on its purpose
| if let urlString = webView.backForwardList.backItem?.url.absoluteString, urlString.isSearchWithBang { | ||
| if let item = webView.backForwardList.item(at: -2) { | ||
| return webView.navigator()?.go(to: item) | ||
| } | ||
| } |
There was a problem hiding this comment.
this check doesn‘t belong here as we were struggling to reduce navigation policy decision logics and move it away to TabExtensions
| guard canGoForward else { return nil } | ||
|
|
||
| // If search with bang go forward of 2 spaces (to avoid redirect) | ||
| if let urlString = webView.backForwardList.forwardItem?.url.absoluteString, urlString.isSearchWithBang { |
There was a problem hiding this comment.
these won‘t work for gesture (2-finger swipe) back/forw navigations
3 participants
Task/Issue URL: https://app.asana.com/0/1177771139624306/1204176861755199/f
Description: When navigating back after using a back it gets stuck in a redirection loop. As a hack fix it checks wether the URL we are about to navigate to is a search with bang, if so navigates 2 positions back. The same happens forward.
Also it removes search with bang items from the context menu of the back/forward buttons.
Steps to test this PR:
<!—
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be marked with the
DO NOT MERGElabel (particularly if it's a draft)If it's pending Product Review/PFR, please add the
Pending Product Reviewlabel.If at any point it isn't actively being worked on/ready for review/otherwise moving forward (besides the above PR/PFR exception) strongly consider closing it (or not opening it in the first place). If you decide not to close it, make sure it's labelled to make it clear the PRs state and comment with more information.
—>
—
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation