Skip to content
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 bug where wrong address bar text is shown when opening DuckPlayer… #1434

Merged

Conversation

SabrinaTardio
Copy link
Collaborator

… from target=blank links

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

Description: fix bug where wrong address bar text is shown when opening DuckPlayer from target=blank links. Moves the check for DuckPlayer URLs in the Tab view model before when in case the url is nil the tab content url is associated to the parent tab content url.

Steps to test this PR:

  1. Create an HTML file with the follwing
YouTube 2. Drag and drop it in the browser click on the link and select to watch the video with DuckPlayer. 3. Check that the address bar is empty 4. Go on youtube select a video and chose to play it with DuckPlayer 5. 3. Check that the address bar is empty

<!—
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be marked with the DO NOT MERGE label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the Pending Product Review label.

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

if tab.content.url?.isDuckPlayer ?? false || tab.content.url?.isDuckPlayerScheme ?? false {
return nil
} else {
return tab.content.url ?? tab.parentTab?.content.url
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look correct. What is the motivation behind this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue is that in this check was in Tab userEditableUrl (if you look at the code I removed there) which makes it nil when the url is DuckPlayer related and when it arrives to this point the tab.content.url is nil and when this is nil we use the parent tab url which leads to the bug: instead of showing nothing in the address bar we show the parent tab url.

By moving the check here we make sure that if the url is duck player related the tabUrl is nil otherwise we try to use the parent tab url (which we don’t want in case of duck player)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A discussed I removed the patent tab url completely… Hopefully we will not see anything weird as result. I asked Sam and even he found it before that PR and doesn’t know why it is there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @mallexxx would know, but he's AFK right now.

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅ I bet we will discover in few weeks why the parent URL is there. 😬

@SabrinaTardio SabrinaTardio merged commit a3a1a96 into develop Aug 10, 2023
11 checks passed
@SabrinaTardio SabrinaTardio deleted the sabrina/bug-Wrong-address-bar-text-for-DuckPlayer branch August 10, 2023 10:09
samsymons added a commit that referenced this pull request Aug 13, 2023
# By Dominik Kapusta (5) and others
# Via GitHub (1) and Lorenzo Mattei (1)
* develop:
  Add support for syncing Credentials (#1368)
  Fix dsym upload to S3
  Bump version to 1.51.1 (48)
  Report failed unit tests to Asana (#1457)
  Use MainActor to ensure FutureExtension tests are dispatched on main queue (#1472)
  Move Make /Applications symlink build phase to later (#1470)
  don't show day 7 when day 0 has been dismissed (#1468)
  Update BSK with autofill 8.1.2 (#1471)
  Add job to create a task on asana if tests fails on develop (#1461)
  Disables a flaky test from App Store unit tests (#1467)
  Fix an autoconsent exception on old Safari (#1460)
  fix bug where wrong address bar text is shown when opening DuckPlayer… (#1434)
  Fix FutureExtensionTests (#1450)
  Add pixel to determine Bitwarden use on Mac (#1412)
  Skip /Applications symlink script for Debug builds (#1443)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Aug 13, 2023
* develop:
  Update NetP endpoint (#1459)
  Add support for syncing Credentials (#1368)
  Fix dsym upload to S3
  Bump version to 1.51.1 (48)
  Report failed unit tests to Asana (#1457)
  Use MainActor to ensure FutureExtension tests are dispatched on main queue (#1472)
  Move Make /Applications symlink build phase to later (#1470)
  don't show day 7 when day 0 has been dismissed (#1468)
  Update BSK with autofill 8.1.2 (#1471)
  Add job to create a task on asana if tests fails on develop (#1461)
  Disables a flaky test from App Store unit tests (#1467)
  Fix an autoconsent exception on old Safari (#1460)
  fix bug where wrong address bar text is shown when opening DuckPlayer… (#1434)
  Fix FutureExtensionTests (#1450)
  Add pixel to determine Bitwarden use on Mac (#1412)
  Encapsulate NetP subjects within observers (#1436)
  Skip /Applications symlink script for Debug builds (#1443)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants