-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
bugfix: Fix Temporary Containers Automatic mode | change firefox -> zen for default cookiestore id #2411
base: dev
Are you sure you want to change the base?
bugfix: Fix Temporary Containers Automatic mode | change firefox -> zen for default cookiestore id #2411
Conversation
Id have to investigate this, as idk if it'll log out of all session for all users updating |
Based off a quick test, when using the tab without a container, I don't get logged out of accounts when I change from |
Hi, Thanks for all your hard work! |
This PR would break Simple Tab Groups automatic temporary container functionality as it checks against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
this worked to solve the problem reported here #172 (I tested locally)
can this be merged please @mr-cheff ? |
Does this break the Firefox Multi-Account Containers extension? The extension is essential for properly syncing containers together with workspaces. |
I personally haven't observed any breaking behaviour, but my testing hasn't been thorough. Looking at the code of Multi-Account Containers it doesn't look like it breaks anything. Every new "no container" tab is counted as a container tab, but that's just a fun stat counter. It may impact Container remote syncing used by MAC, but I don't use that feature and I wouldn't know what behaviour change to observe. In regards to behaviours when interacting with the Temporary Containers extension, if a workspace has a default container set, that default container will be used upon opening a new tab instead of a temporary container(assuming Temporary Containers was in Automatic mode). For people who want to fix Temporary Containers Automatic Mode for Zen without this patch you can do the following:
|
75bd3ed
to
a74f878
Compare
Closes #172
This PR fixes automatic mode for Temporary Containers by patching
firefox-default
tozen-default
. I also included changes to the other store names to maintain consistency.Based off brief testing this does not impact Workspaces. The default container of a workspace takes precedence over an automatic container generated say by Temporary Containers.
I can't comment whether this fix breaks other container extensions. If they hardcoded checking cookieStoreId against
firefox-default
, then this fix would break them.Why temporary containers doesn't work
Inspecting the Temporary Containers extension and then opening a new tab, you will notice an uncaught error from
showOrHide
caused by L4943.Examining Temporary Containers source code,
showOrHide
is found inpageaction.ts
with the line causing the throw at L43. The correct behaviour is theif else
on L31 executing, not the else.In Zen that
if else
doesn't execute because Temporary Container assumes the default cookieStoreId is the browser's name (zen
) appended with-default
. However the default cookiestore id is set tofirefox-default
. Thus theelse if
doesn't execute aszen-default
does not equalfirefox-default
and the extension proceeds to theelse
causing the error.More generally if the above error did not occur, Automatic mode would still not would as the necessary steps would not trigger here(for on created automatic mode) or would return early here(for on request Automatic Mode) as it would be looking for
zen-default
when the value is actuallyfirefox-default
.Breaks:
firefox-default