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

Closing empty tabs after download #2510

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Conversation

tomasstrba
Copy link
Contributor

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

Description:
Fix of empty tabs remaining in the tab bar after a download finishes.

Steps to test this PR:
Test downloads which open a new tab are closed

  1. Visit this task
  2. Click on the download icon on the first image
  3. Make sure the new tab is closed after download finishes
  4. Click on the same image again
  5. Make sure the new tab is closed after download finishes

Test downloads which doesn't open a new tab

  1. Visit this task
  2. Click on the image to start the download
  3. Make sure the current tab is not closed

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 27, 2024
@tomasstrba tomasstrba removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 27, 2024
@duckduckgo duckduckgo deleted a comment from github-actions bot Mar 27, 2024
@tomasstrba tomasstrba requested a review from mallexxx March 27, 2024 08:34
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

@tomasstrba, unfortunately this is not a completely correct implementation: we‘re trying to close the web view for any navigation response (navAction == nil) converted to download, the web view won‘t close here only because it wasn‘t created by js (you can see the error message in js console)

I suggest the following change:

        var isMainFrameNavigationActionWithNoHistory: Bool {
            guard let navigationAction,
                  navigationAction.isForMainFrame,
                  navigationAction.isTargetingNewWindow,
                  // webView has no navigation history (downloaded navigationAction has started from an empty state)
                  (navigationAction.redirectHistory?.first ?? navigationAction).fromHistoryItemIdentity == nil
            else { return false }
            return true
        }
        guard let webView = download.webView,
              isMainFrameNavigationActionWithNoHistory
                // if converted from navigation response but no page was loaded
                || navigationAction == nil && webView.backForwardList.currentItem == nil else { return }

@tomasstrba
Copy link
Contributor Author

Thank you, @mallexxx! Unfortunately, this solution doesn't work for this scenario:

  1. Open https://app.asana.com/0/1142021229838617/1206919179674977/f
  2. Repeatedly download images using the download button
Screenshot 2024-04-03 at 10 37 23 AM

Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

There was an unintended code duplication, I‘ve fixed it,
Validated the tests steps: LGTM

Copy link
Contributor Author

@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.

I tested it too and it works as expected 💯 Thank you for the better fix, @mallexxx!

@tomasstrba tomasstrba merged commit b484b1e into main Apr 3, 2024
17 checks passed
@tomasstrba tomasstrba deleted the tom/empty-tabs-after-download branch April 3, 2024 12:35
samsymons pushed a commit that referenced this pull request Apr 5, 2024
Task/Issue URL:
https://app.asana.com/0/1177771139624306/1206835038894671/f

**Description**:
Fix of empty tabs remaining in the tab bar after a download finishes.
samsymons added a commit that referenced this pull request Apr 8, 2024
* main: (338 commits)
  Fixes the VPN restarting logic on update (#2545)
  fix download save panel disappearing on navigation (#2549)
  Add CI support for handling installation attribution (#2502)
  Bump version to 1.82.0 (152)
  Pixel changed in subscription (#2541)
  add LSHandlerRank for .duckload document type (#2537)
  Fix usertext comment to ensure it matches localizable string (#2546)
  Update Neighbor Report (#2542)
  DBP: Compare by url and not name (#2544)
  DBP: Compare by url and not name (#2544)
  Adds a series of UI tests for Bookmarks Bar visibility
  Improve Handling of noData Import Errors (#2494)
  Bump version to 1.82.0 (151)
  Use the default action button style for VPN onboarding (#2529)
  Redirect from purchase page to macOS native purchase flow (#2538)
  Closing empty tabs after download (#2510)
  Add Web UI loading state pixels (#2531)
  fix localization warnings (#2288)
  Select URL in address bar when editing and use cmd+L (#2536)
  Add attemptId to Captcha /result endpoint (#2530)
  ...
samsymons added a commit that referenced this pull request Apr 8, 2024
* main: (22 commits)
  Removes last instance of NETWORK_PROTECTION flag (#2573)
  Bye bye NETWORK_PROTECTION (#2509)
  QWD: Enable Hide/Show for Autofill Credit Card Number and CVV (#2539)
  Removed the VPN waitlist beta pixels (#2555)
  VPN: Cleanup authorize call (#2565)
  Revert "VPN: Cleanup authorize call (#2553)"
  VPN: Cleanup authorize call (#2553)
  Improves underlying error pixel information (#2543)
  Fixes the VPN restarting logic on update (#2545)
  fix download save panel disappearing on navigation (#2549)
  Add CI support for handling installation attribution (#2502)
  Fix usertext comment to ensure it matches localizable string (#2546)
  Update Neighbor Report (#2542)
  DBP: Compare by url and not name (#2544)
  Adds a series of UI tests for Bookmarks Bar visibility
  Improve Handling of noData Import Errors (#2494)
  Use the default action button style for VPN onboarding (#2529)
  Closing empty tabs after download (#2510)
  Add Web UI loading state pixels (#2531)
  fix localization warnings (#2288)
  ...
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