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

Remember choice for default containers in the "Always open in" confirm page #2649

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

rafeerahman
Copy link
Contributor

@rafeerahman rafeerahman commented Jul 18, 2024

Before submitting your pull request

  • I agree to license my code under the MPL 2.0 license.
  • I rebased my work on top of the main branch.
  • I ran npm test and all tests passed.
  • I added test coverages if relevant.

Description

When a user is on the confirm-page after re-opening a site they chose to 'always open in a container', if they click "Remember my choice" and then "Open in current tab" (default container), this is not remembered. i.e. The next time they open the site, they are prompted to this screen again.

This PR fixes that.

image

How to test

(Remember choice for default container)

  • Go to youtube.com
  • In the browserAction menu, choose "Always Open This Site in..."
  • Choose the work container
  • Open youtube.com in a new tab.
  • On the confirmation page prompt, check "Remember my decision for this site" then click "Open in current tab"
  • Open youtube.com in a new tab, and verify that there is no confirmation page. (if you want to put it in a container again, click "Always Open This Site In....")
  • In the work containers "site list" verify that it is empty.

(Switching between two containers)

  • Go to youtube.com
  • In the browserAction menu, choose "Always Open This Site in..."
  • Choose the Work container, youtube will open in this container
  • In the browserAction menu, choose "Always Open This Site in..."
  • Choose the Personal container
  • On the confirmation page, check "remember my decision for this site".
  • Choose the "Work" container (the option on the left).
  • Verify that the choice is remembered.

Type of change

Select all that apply.

  • Bug fix
  • New feature
  • Major change (fix or feature that would cause existing functionality to work differently than in the current version)

Tag issues related to this pull request:

@rafeerahman rafeerahman force-pushed the 2263-always-open-in-container-bug branch 4 times, most recently from 402828f to c48a75a Compare July 18, 2024 16:21
@rafeerahman
Copy link
Contributor Author

Also looking into integrating / fixing #2617, which tries to remember the choice when switching to a container that is not the "default container" (i.e. not "Open in current tab").

Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

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

Couple blocking changes.

Can you write out the steps you used to spot-check this? I tried:

  1. Go to accounts.firefox.com
  2. In the browserAction menu, choose "Always Open This Site in..."
  3. In the list of containers, choose "Work"
  4. Close the tab
  5. Open a new window
  6. Go to accounts.firefox.com

Expected Result:

  • I should be prompted to confirm I want to open the site in a Work tab

Actual Result:

  • I'm not prompted to confirm I want to open the site in a Work tab

src/js/background/assignManager.js Outdated Show resolved Hide resolved
src/js/background/assignManager.js Outdated Show resolved Hide resolved
@groovecoder
Copy link
Member

Also, please change the commit message to start with fix #2603: so that GitHub will automatically link this PR with the corresponding issue.

@rafeerahman
Copy link
Contributor Author

Couple blocking changes.

Can you write out the steps you used to spot-check this? I tried:

1. Go to accounts.firefox.com

2. In the browserAction menu, choose "Always Open This Site in..."

3. In the list of containers, choose "Work"

4. Close the tab

5. Open a new window

6. Go to accounts.firefox.com

Expected Result:

* I should be prompted to confirm I want to open the site in a Work tab

Actual Result:

* I'm not prompted to confirm I want to open the site in a Work tab

This seems to be a bug in the production version as well. On the firefox accounts website, I get this error both locally and in prod,
image.

Heres how I spot checked:

  1. Go to youtube.com
  2. In the browserAction menu, choose "Always Open This Site in..."
  3. Choose the work container
  4. Open youtube.com in a new tab.
  5. On the confirmation page prompt, check "Remember my decision for this site" then click "Open in current tab"
  6. Open youtube.com in a new tab, and verify that there is no confirmation page. (if you want to put it in a container again, click "Always Open This Site In....")
  7. In the work containers "site list" verify that it is empty.

Other checks (switching between two containers):

  1. Go to youtube.com
  2. In the browserAction menu, choose "Always Open This Site in..."
  3. Choose the work container, youtube will open in this container
  4. In the browserAction menu, choose "Always Open This Site in..."
  5. Choose the Personal container
  6. Verify that the existing behavior is valid when choosing a container or remembering the choices.

@rafeerahman rafeerahman force-pushed the 2263-always-open-in-container-bug branch from c48a75a to 5164527 Compare July 23, 2024 15:40
@rafeerahman rafeerahman force-pushed the 2263-always-open-in-container-bug branch from 5164527 to cd343ab Compare July 23, 2024 15:42
Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

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

Thanks @rafeerahman I was able to reproduce the bug and the fix on youtube.com.

There's still one more bug that I think is worth fixing:

  1. Go to youtube.com
  2. In the browserAction menu, choose "Always Open This Site in..."
  3. Choose the work container
  4. Open youtube.com in a new tab.
  5. On the confirmation page prompt, check "Remember my decision for this site" then click "Open in current tab"
  6. Open youtube.com in a new tab
    • verify that there is no confirmation page.
    • verify youtube.com opens in default/non-container tab
  7. Open the work containers "site list"
    • verify that it is empty.
  8. Open a new work tab
  9. Go to youtube.com

Expected results:
youtube.com automatically opens in default/non-container tab without prompting

Actual results:
youtube.com opens in work container tab

@groovecoder groovecoder linked an issue Aug 1, 2024 that may be closed by this pull request
@rafeerahman rafeerahman force-pushed the 2263-always-open-in-container-bug branch from 55eb69d to 0e37cb7 Compare August 23, 2024 16:17
Comment on lines 84 to 92
if (neverAsk && !currentContainer) {
await browser.runtime.sendMessage({
method: "neverAsk",
neverAsk: true,
defaultContainer: true,
cookieStoreId: currentCookieStoreId,
pageUrl: redirectUrl
});
}

if (neverAsk) {
browser.runtime.sendMessage({
method: "neverAsk",
neverAsk: true,
cookieStoreId: currentCookieStoreId,
pageUrl: redirectUrl
});
}

Choose a reason for hiding this comment

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

Maybe optimize it by something like:

if (neverAsk) {
  await browser.runtime.sendMessage({
    method: "neverAsk",
    neverAsk: true,
    cookieStoreId: currentCookieStoreId,
    pageUrl: redirectUrl,
    defaultContainer: !currentContainer
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@groovecoder groovecoder merged commit 077c7e0 into main Aug 27, 2024
2 checks passed
@groovecoder groovecoder deleted the 2263-always-open-in-container-bug branch August 27, 2024 18:48
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.

Always Open Site In Container cannot be disabled once engaged
3 participants