Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Disable in private browsing mode #3120

Closed
jaredhirsch opened this issue Jul 13, 2017 · 11 comments
Closed

Disable in private browsing mode #3120

jaredhirsch opened this issue Jul 13, 2017 · 11 comments
Assignees

Comments

@jaredhirsch
Copy link
Member

Looks like the incognito manifest key should work

@jaredhirsch jaredhirsch self-assigned this Jul 13, 2017
@ianb ianb mentioned this issue Jul 13, 2017
2 tasks
@jaredhirsch
Copy link
Member Author

turns out incognito manifest key does nothing

@jaredhirsch
Copy link
Member Author

lol, browser compatibility table here shows that 'incognito' is not yet supported by firefox https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/incognito

@jaredhirsch
Copy link
Member Author

aha! tab.incognito tells us

@jaredhirsch
Copy link
Member Author

@johngruen Turns out the 'incognito' manifest key only works on chrome, for now.

We can eagerly or lazily detect private browsing. Since Screenshots, in the past, caused Talos regressions by running code when the user switches tabs, I think lazy detection is best. This means we'd not disable the button in private browsing mode, but instead would show an error when the user initiates a shot.

How do you want to message the private browsing limitation to users? It seems late in the 55 cycle to add a string

@johngruen
Copy link
Contributor

johngruen commented Jul 13, 2017

@_6a68 I don't see how we do this w/o a string...

title=Private Browsing does not support Screenshots
subtitle=Sorry for the inconvenience. We are working on this feature for future releases.

If we can push to dev today we should get significant coverage over the weekend.

@jaredhirsch
Copy link
Member Author

👍

@ianb
Copy link
Contributor

ianb commented Jul 13, 2017

Hmm... we haven't updated strings yet in 10.x.0. To get strings in we'll need to figure out how to cherry-pick them all out, or maybe just export this one string?

@jaredhirsch
Copy link
Member Author

I'll add a list of strings and dates to #3121 and we can see which ones need to land

@ianb ianb closed this as completed in aefc639 Jul 13, 2017
ianb added a commit that referenced this issue Jul 13, 2017
@Caspy7
Copy link

Caspy7 commented Jul 13, 2017

Why do we need to disable in private mode? We're only sending back data if the user interacts with it, no?
So Screenshots is on the level with saving bookmarks. They're using instantiated.

@Caspy7
Copy link

Caspy7 commented Jul 13, 2017

I have read the comments in issue #2818 and believe I understand that this may not be a permanent change, but is to protect privacy until a better mitigation is implemented.

@dveditz
Copy link

dveditz commented Jul 14, 2017

Clearly it would be important to remind users they're in private mode in case they forgot, but I hope that the functionality is eventually restored. Even the server upload could be appropriate for some use cases: say I'm shopping for rings and don't want local traces, but want to share a screenshot to get advice. Random link, no history in private mode, and delete the server copy after and it's private enough.

jaredhirsch pushed a commit that referenced this issue Jul 24, 2017
* add cloud icon

* Fix #2981, sanitize download filename more fully
This adds : (important on Windows), \, <, and > to the blacklist.
Followup in #3083

* add context fill icons

* Change version to 10.4.0 with changelog

* Fix Bug 1373614, stop the embedded WebExtension unconditionally
In test conditions, when the browser is started and stopped very quickly, sometimes we didn't shut down the WebExtension because it hadn't fully started.
r=kmag (in https://bugzilla.mozilla.org/show_bug.cgi?id=1373614)

* Fix #3120, disable Screenshots in private windows

* Update version to 10.5.0 with changelog

* Iframe tests (#3134)

* Validate iframe URLs
Remove unneeded iframe onload handlers

* Put temporary clipboard TEXTAREA in an iframe
With iframe URL validation

* Update version to 10.6.0 with changelog

* Update addon export branch to 10.7.0 (#3143)

* set dimensions for icon and add to startup (#3136)

* Address 10.6 review comments (bug 1381132)

* Update version to 10.7.0 with changelog

* Element fix (#3157)

* Do not re-wrap onResize when adding and removing the listener
This caused the removeEventListener to silently fail, as the wrapped functions did not match (by identity)
Fixes #3153

* Suppress error popups for all errors in resize handlers
Errors still go to Sentry

* Fix #3135, update privacy notice URL

* Fix #3135, update privacy notice URL (#3158)

* Update version to 10.8.0 with changelog
jaredhirsch pushed a commit that referenced this issue Aug 4, 2017
* add cloud icon

* Fix #2981, sanitize download filename more fully
This adds : (important on Windows), \, <, and > to the blacklist.
Followup in #3083

* add context fill icons

* Change version to 10.4.0 with changelog

* Fix Bug 1373614, stop the embedded WebExtension unconditionally
In test conditions, when the browser is started and stopped very quickly, sometimes we didn't shut down the WebExtension because it hadn't fully started.
r=kmag (in https://bugzilla.mozilla.org/show_bug.cgi?id=1373614)

* Fix #3120, disable Screenshots in private windows

* Update version to 10.5.0 with changelog

* Iframe tests (#3134)

* Validate iframe URLs
Remove unneeded iframe onload handlers

* Put temporary clipboard TEXTAREA in an iframe
With iframe URL validation

* Update version to 10.6.0 with changelog

* Update addon export branch to 10.7.0 (#3143)

* set dimensions for icon and add to startup (#3136)

* Address 10.6 review comments (bug 1381132)

* Update version to 10.7.0 with changelog

* Element fix (#3157)

* Do not re-wrap onResize when adding and removing the listener
This caused the removeEventListener to silently fail, as the wrapped functions did not match (by identity)
Fixes #3153

* Suppress error popups for all errors in resize handlers
Errors still go to Sentry

* Fix #3135, update privacy notice URL (#3158)

* Update version to 10.8.0 with changelog

* Remove full-page & visible buttons and remove from onboarding

* Update version to 10.9.0 with changelog

* Don't localize removed buttons

* Tweak button container CSS

* Synchronize startup code more carefully; hopefully fixes #3257

* Update version to 10.10.0 with changelog

* Revert "Remove full-page & visible buttons and remove from onboarding"

This reverts commit 07a27a8.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants