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

Add 'Open in New Tab' support for DuckPlayer #3431

Merged
merged 12 commits into from
Oct 15, 2024
Merged

Conversation

afterxleep
Copy link
Collaborator

@afterxleep afterxleep commented Oct 11, 2024

Task/Issue URL: https://app.asana.com/0/1204099484721401/1208366641490350/f

Description:
Adds a setting to allow users to open DuckPlayer videos in a new tab. (On by default)

Steps to test this PR:

  • Make yourself internal user
  • Go to Settings > All Debug Settings, and Tap "Override DuckPlayer Experiment (Experiment)
  • Go to Settings > DuckPlayer, Set it to 'Always', and Enable "Open Duck Player in a new tab"

SERP

  • Open a new tab
  • Search for 'metallica videos'
  • Tap a video
  • Should open in new tab
  • Tap 'Back'
  • New tab should be closed

SERP Videos Vertical

  • Open a new tab
  • Search for 'metallica videos'
  • Go to 'Videos'
  • Tap a video
  • Should open in new tab
  • Tap 'Back'
  • New Tab should be closed

Youtube

  • Open a new tab
  • Go to Youtube.com
  • Search for 'metallica videos'
  • Tap a video
  • Should open in new tab
  • Tap 'Back'
  • New Tab should be closed

Other Page (Javascript link)

Other Page (Direct link)

Direct Navigation

  • Open a new tab
  • Go to duck://player/WM8bTdBs-cw
  • DuckPlayer should load in the same tab

Always ask mode


Copy link

github-actions bot commented Oct 11, 2024

Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against d59d700

@afterxleep afterxleep marked this pull request as ready for review October 11, 2024 18:04
@afterxleep afterxleep requested a review from Bunn October 11, 2024 18:05
@Bunn
Copy link
Contributor

Bunn commented Oct 14, 2024

LGTM so far, the only issue I can see is:

  • Make sure you have new tab on, and DP enabled
  • Open duck://player/WM8bTdBs-cw
  • Click on Watch on youtube
  • Instead of opening on youtube, it opens DP again
WatchYoutube.mov

Other than that, I saw one issue I can't reproduce again:

  • Make sure you have new tab on, and DP as ask
  • Open youtube.com search for a video
  • Open a video
  • In the overlay, click on Turn On DP
  • Click Back
  • At this moment, I couldn't interact with the website anymore. Reloading fixed it.

@afterxleep
Copy link
Collaborator Author

afterxleep commented Oct 14, 2024

@Bunn Pushed this change, which should fix the scenario you've seen.

The issue was that Youtube started performing a redirect after the page loads and removes the parameter we use to "Watch in Youtube", which triggers DuckPlayer to be shown again.

Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

@afterxleep afterxleep merged commit f876b4d into main Oct 15, 2024
20 checks passed
@afterxleep afterxleep deleted the daniel/dp.new.tab.base branch October 15, 2024 12:57
samsymons added a commit that referenced this pull request Oct 16, 2024
* main:
  Add Events Firing for Phishing Detection Settings: Point to BSK (#3423)
  DuckPlayer: Temporary Fix for Watch In Youtube (#3437)
  Add 'Open in New Tab' support for DuckPlayer (#3431)
  update BSK dependency (#3434)
samsymons added a commit that referenced this pull request Oct 21, 2024
# By Daniel Bernal (4) and others
# Via Daniel Bernal (1) and others
* main:
  Pixel retrying (#3358)
  Remove `voiceSearchHelper` from `AppDependencyProvider` (#3452)
  Update AutoClearSettingsViewController to use DI for app settings (#3448)
  Bump BSK (#3441)
  Remove `SubscriptionFeatureAvailability` from `AppDependencyProvider` (#3447)
  Release 7.141.0-2 (#3451)
  Do not notify the FE on experiment activation (#3450)
  point to bsk branch (#3444)
  bump bsk for content blocker rules fix (#3445)
  speculative fix for set bars visibility crashes (#3442)
  Release 7.141.0-1 (#3443)
  Fix browsing menu bottom offset when bar location set to bottom (#3440)
  Properly handle responses that should trigger download action (#3407)
  Add Events Firing for Phishing Detection Settings: Point to BSK (#3423)
  DuckPlayer: Temporary Fix for Watch In Youtube (#3437)
  Add 'Open in New Tab' support for DuckPlayer (#3431)
  update BSK dependency (#3434)
  Release 7.141.0-0 (#3435)
  Add error handling to contrainer removal (#3424)
  Prevent autofill prompt crash for edge case where a context menu is also visible on screen (#3417)

# 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants