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

#303: Reset cookies in site manager #2654

Merged
merged 5 commits into from
Aug 28, 2024
Merged

#303: Reset cookies in site manager #2654

merged 5 commits into from
Aug 28, 2024

Conversation

rafeerahman
Copy link
Contributor

@rafeerahman rafeerahman commented Aug 1, 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

Allows users to reset cookies for a site in the containers "Site manager"

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)

How to test:

Site manager cookie clearing:

  1. cd into src/_locales and pull the latest changes. Then checkout the branch titled clear-storage-confirmation-strings
  2. Go to relay.firefox.com and click the "Always open in container" icon then select your Personal container.
  3. Sign in to your Relay account.
  4. Open the MAC popup.
  5. Click the right arrow on the "Personal" container, then click "Manage this container"
  6. Click on "Manage site list"
  7. Verify that you can hover over and see tooltips for the delete and refresh icons.
  8. Click on the refresh icon.
  9. Verify that the permissions popup shows if you don't have "Clear recent browsing history, cookies, and related data" toggled already, and click "Allow".
  10. Go back to the refresh icon, and click it to clear cookies.
  11. Verify a popup notifcation confirming that cookies have been cleared is shown.
  12. Open relay.firefox.com again and check that you are not signed in.

Container storage clearing:

  1. Disable "Clear recent browsing history, cookies, and related data" permissions in the extension manager.
  2. Open one a site of your choice in a container.
  3. Open that container's menu by clicking the "right arrow" on it in the popup.
  4. Click "clear storage and cookies".
  5. Verify that the permission pop up shows, and click "Allow".
  6. Go Back to the "Clear storage and cookies" option and click it.
  7. Verify that the confirmation page shows up.
  8. Verify that clicking "OK" clears cookies and localstorage.
  9. Verify that this pages "Cancel" and "Back" options work.

Tag issues related to this pull request:

L10n: mozilla-l10n/multi-account-containers-l10n#28
L10n: mozilla-l10n/multi-account-containers-l10n#29

@rafeerahman rafeerahman linked an issue Aug 1, 2024 that may be closed by this pull request
@rafeerahman rafeerahman force-pushed the feat-#303 branch 2 times, most recently from dfcbe63 to fea2c87 Compare August 1, 2024 21:51
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 this is a great feature. After re-reading the issue and comments, this doesn't quite seem to implement what most people are asking for.

It seems most people want a way to clear all the browsing data (cookies, cache, and local storage) for the entire container. It seems @dannycolin's PR is quite close to that already.

@dannycolin: Can you say more about why your PR is WIP? It seems ready to merge to me.

@rafeerahman: If @dannycolin's PR is ready, I think we should combine both of these PRs together. I think they both add a very nice features for reseting container data without having to destroy the container. If anything, I think you're change is even more advanced and sophisticated!

src/js/background/assignManager.js Outdated Show resolved Hide resolved
src/js/popup.js Outdated Show resolved Hide resolved
@dannycolin
Copy link
Collaborator

@dannycolin: Can you say more about why your PR is WIP? It seems ready to merge to me.

There's a limitation around the cache removal. According to the documentation on MDN, browsingData.removeCache() will ignore the browsingData.RemovalOptions object and so you cannot limit the removal to a specific container.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/removeCache

@rafeerahman: If @dannycolin's PR is ready, I think we should combine both of these PRs together. I think they both add a very nice features for reseting container data without having to destroy the container. If anything, I think you're change is even more advanced and sophisticated!

I Agree. The ability to remove per domain is great for a poweruser tool but it does increase the complexity of the UX/UI. I don't have a strong opinion on implementing it or not but I simply wanted to point it out since our frontend code is in need of some TLC :).

@chetan-reddy
Copy link

@dannycolin: Can you say more about why your PR is WIP? It seems ready to merge to me.

There's a limitation around the cache removal. According to the documentation on MDN, browsingData.removeCache() will ignore the browsingData.RemovalOptions object and so you cannot limit the removal to a specific container.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/removeCache

@dannycolin I apologize if I'm wrong, but would you be able to use browsingData.remove() which does support limiting by RemovalOptions.cookieStoreId and browsingData.DataTypeSet ?

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browsingData/remove

I inferred that browsingData.remove might work from this comment -> https://bugzilla.mozilla.org/show_bug.cgi?id=1353726#c13

@dannycolin
Copy link
Collaborator

dannycolin commented Aug 6, 2024

@dannycolin I apologize if I'm wrong, but would you be able to use browsingData.remove() which does support limiting by RemovalOptions.cookieStoreId and browsingData.DataTypeSet ?

I inferred that browsingData.remove might work from this comment -> https://bugzilla.mozilla.org/show_bug.cgi?id=1353726#c13

@chetan-reddy Excerpt from the webextension API documentation itself:

With browsingData.removeCache() note that although this function can take a browsingData.RemovalOptions object, it will be ignored. The entire cache is always cleared when using this function.

@jkuj6
Copy link

jkuj6 commented Aug 13, 2024

I came here from cookie autodelete, having removed it because its no longer maintained.
I was about to request a feature in here regarding removing or "emptying out" a container's entire cookies and cache by the push of a button. Is this whats being implemented here?
Maybe when long-pressing the "open a new tab +" next to each container there can be a Brush Cleaning icon button to clear the entire container and have a message popup telling you the container's data (without closing any tabs) has been cleared?

thanks in advance

@rafeerahman
Copy link
Contributor Author

I came here from cookie autodelete, having removed it because its no longer maintained. I was about to request a feature in here regarding removing or "emptying out" a container's entire cookies and cache by the push of a button. Is this whats being implemented here? Maybe when long-pressing the "open a new tab +" next to each container there can be a Brush Cleaning icon button to clear the entire container and have a message popup telling you the container's data (without closing any tabs) has been cleared?

Yes thats whats being implemented. Cool idea, we can definitely look into adding it in that menu as well.

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.

change (blocking): I'm afraid that clearing all site data is destructive enough that we should add a confirmation dialog to this? Or change the UI so that it's harder to accidentally do it.

The clear cookies UI isn't too bad ... the button is next to another data-destroying button so it's less likely that someone would accidentally click it. But it's still possible, so could we move it further away from the other button?
image

More concerning is how close the "Clear storage and cookies" item is to "Always open site in Container" item - which are 2 very different intentions.
image

I'm asking @maxxcrawford if there's a Firefox design guide to help us here.

src/js/background/assignManager.js Outdated Show resolved Hide resolved
src/js/background/backgroundLogic.js Outdated Show resolved Hide resolved
src/manifest.json Outdated Show resolved Hide resolved
src/js/popup.js Show resolved Hide resolved
@dannycolin
Copy link
Collaborator

@groovecoder For the confirmation dialog, I think we should use the same pattern that we use when a user deletes a container.

@andsofine
Copy link

Sorry, I don't know if this is the right place, but I was wondering if these changes will be implemented in the browser itself?

Cause cookies can be deleted through the lock icon (to the left of the address bar) and also in the settings.

I was clearing cookies through the lock icon for a while until I realized that it deletes cookies for all containers. It wasn't very obvious

@groovecoder
Copy link
Member

Sorry, I don't know if this is the right place, but I was wondering if these changes will be implemented in the browser itself?

No. You would need to file a new bugzilla bug in the Data Sanitization component change the behavior of the lock icon item.

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.

suggestion (non-blocking): This is an existing bug, so it shouldn't block this feature. It's just more obvious with this new feature.

For some reason, the .scrollable block-size: 100% directive causes the site assignment panel to ALWAYS have a scrollbar even when its not needed?

image

And the trash can icon is covered up by the scrollbar.

I couldn't find an existing bug for this, so I filed a new one here: #2662

@groovecoder groovecoder merged commit a53eb64 into main Aug 28, 2024
2 checks passed
@groovecoder groovecoder deleted the feat-#303 branch August 28, 2024 16:27
@andsofine
Copy link

Sorry, I don't know if this is the right place, but I was wondering if these changes will be implemented in the browser itself?

No. You would need to file a new bugzilla bug in the Data Sanitization component change the behavior of the lock icon item.

It says that my email address will be public, is there any other issue tracker?
Or maybe can you fill it?

@colans
Copy link

colans commented Sep 3, 2024

@andsofine : Use an email alias. There are various options, but I like https://proton.me/pass/aliases

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.

Reset cache/cookies of containers without deleting them
7 participants