Skip to content

Add option to whitelist domain from containment #108

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

Merged
merged 2 commits into from
Dec 9, 2020

Conversation

sents
Copy link
Contributor

@sents sents commented Nov 24, 2020

This pull request adds an option to include a list of whitelisted domains.
It is my first real pull request so I hope I am doing this right.

Note that this would fix #104.

@Perflyst
Copy link
Collaborator

Thanks! I will test your patch as soon as possible. Also it needs a quick rebase

@hackerncoder what do you think?

@sents
Copy link
Contributor Author

sents commented Nov 25, 2020

I rebased it now.

I've noticed that pressing enter in the extension preferences triggers saving the preferences.
As the domains are separated by newlines there is no way to enter multiple domains right now.
I would propose to use "," or ";" as separator. Both characters are not allowed as part of domain names.

Before we used a simple text input, which wasn't able to contain
multiple lines
@sents
Copy link
Contributor Author

sents commented Nov 25, 2020

The whitelist option uses a textarea element now, so it is possible to enter multiple domains.

@hackerncoder
Copy link
Collaborator

Thanks! I will test your patch as soon as possible. Also it needs a quick rebase

@hackerncoder what do you think?

This is some serious code, and somewhat out of my ballpark (especially since I don't have that much energy right now), but seems fine.

@hackerncoder
Copy link
Collaborator

I tried it a little (google.com and blogger.com) and it seems fine.

@Perflyst Perflyst merged commit 0989038 into containers-everywhere:master Dec 9, 2020
@Perflyst
Copy link
Collaborator

Perflyst commented Dec 9, 2020

@sents after hackerncoder's test I merged and released this feature. the containment is broken if the whitelisted google urls are empty -> by default the containment is broken

@Perflyst
Copy link
Collaborator

Perflyst commented Dec 9, 2020

Additionally your popup is broken if the preferences are saved with empty whitelist
unknown

@hackerncoder
Copy link
Collaborator

the containment is broken if the whitelisted google urls are empty -> by default the containment is broken

I can't reproduce this.

@sents
Copy link
Contributor Author

sents commented Dec 9, 2020

I'll look into it today.

@sents
Copy link
Contributor Author

sents commented Dec 9, 2020

Unfortunately I am not able to reproduce the bugs relating to the empty whitelist. I've tried my standard profile and a fresh profile without any other extensions.

@Perflyst
Copy link
Collaborator

Perflyst commented Dec 9, 2020

In case you downloaded from AMO - I disabled version 1.5.2 there. If you download https://github.com/containers-everywhere/contain-google/releases/download/1.5.2/google_container-1.5.2.zip, create a fresh profile, use debug addons -> load temporary addon you should be able to reproduce it.

@sents
Copy link
Contributor Author

sents commented Dec 9, 2020

I installed it on a different computer with Firefox 82 and could reproduce the bug. For some reason whitespace gets inserted into the setting when loading the settings for the first time. Thank you for your tip! I will now look into this.

@sents
Copy link
Contributor Author

sents commented Dec 9, 2020

After the fix of #109 leaving the whitelist empty doesn't cause any problems anymore.
The problem was caused by the variables relating to whitelist being undefined, when loading the extension for the first time.
The checkboxes are also undefined, but this doesn't cause any problems because undefined works with boolean operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Exclude accounts.google.com
3 participants