Fix handling external schemes in address bar #493
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ayoy
force-pushed
the
dominik/external-schemes-in-address-bar
branch
from
08045d9 to
9c991d4
Compare
mallexxx
requested changes
Mar 31, 2022
There was a problem hiding this comment.
Looks good overall, but one test case fails:
- Enter mailto:[email protected], submit: confirmation popover appears
- Enter define:foo, submit, SERP for "mailto:[email protected]" is opened
This may be a privacy concern sharing some private data to the search
Scenario: 1. Enter `mailto:[email protected]`, submit: confirmation popover appears 2. Without reloading the page, enter a search phrase in the address bar 3. The search is performed for `mailto:[email protected]` This is caused by PermissionAuthorizationQuery deinitializer which calls decisionHandler that ends up requesting a search for the external URL. The query is deinitialized when the new search phrase is submitted and the sequence of events is following: 1. make search URL for the search phrase 2. make search URL for the external URL from a deinitialized PermissionAuthorizationQuery The latter overrides the search phrase and that's how the bug is observed. I updated PermissionModel.queryAuthorization to not call its decision handler at all when decision from query is `.deinitialized`.
Verify that when the query is reset (deinitialized) then decision handler is not called at all.
…ndler:) to make SwiftLint happy
mallexxx
approved these changes
Apr 1, 2022
There was a problem hiding this comment.
LGTM! Wonderful! 🏅
May I ask adding the following changes before you merge in order to improve memory usage:
- add (item as? TabBarViewItem)?.clear() to collectionView:didEndDisplayingItem:forRepresentedObjectAtIndexPath: in TabBarViewController.swift
- add usedPermissions = Permissions() to TabBarViewItem.clear() method
- change clearSubscriptions() method to cancellables.removeAll()
This will clear usedPermissions as soon as Tab is closed and so deallocate the Permission Query without delays
|
@mallexxx is seems that the changes you asked me to make fixed the visual glitch in the address bar 🚀 |
samsymons
added a commit
that referenced
this pull request
Apr 4, 2022
# By Tomas Strba (5) and others # Via GitHub * develop: Refresh the Autofill panel after importing (#496) Require authentication for login export (#494) Observations database clearing improvements (#495) Fix handling external schemes in address bar (#493) New autoconsent rules and move tab cleanup out of onCommitted (#489) Safari CSV Passwords import +LastPass, 1Password (#483) Split preferences into multiple screens and add an option to show full URL (#487) Color of hover label fixed (#497) Add AMP links protection (#488) Enhancements of fire button options (#484) Rename Logins+ to Autofill (#492) new tab page low hanging fruit (#491) Fix threading issues in Autoconsent (#478) Navigation bar improvement: CMD + back or CMD + forward opens a new tab (#486) TextField refactored to TextView in order to make it scrollable (#481) More Xcode cleaning (#485) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/inbox/1201621708108816/1202055192971137/1202055303685720/f
Tech Design URL:
CC:
Description:
The original issue is about
define:foobeing recognized as an external URL (withdefine:scheme) and not triggering a search, but instead opening a blank page. I realized that other external URLs don't work either when typed directly into the address bar. I figured out that it's because of a recently modified code in Tab.swift which allows for handling external URLs (optionally converting them to search URLs if they can't be handled byNSWorkspace) only when the source of a navigation is the main frame. However, when user enters a URL into the address bar, the source of navigation is not main frame either, so I handled it by checking that navigation type is.other(which covers manually input URLs).Steps to test this PR:
define:footriggers a searchsms://+44123123123tries to open Messages.appmailto:[email protected]tries to open a system default Mail apptest://hellotriggers a search, because this scheme is not supported by any app in the systemTesting checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM