Closed Bug 1403661 Opened 3 years ago Closed 3 years ago

Stop Screenshots from activating in private browsing

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: ianbicking, Assigned: ianbicking)

Details

Attachments

(1 file)

No version update for this, we're going to import several small bugs (in parallel) and then bump the version.

This is the fix for this bug: https://github.com/mozilla-services/screenshots/issues/3491

And is an export of this commit: https://github.com/mozilla-services/screenshots/commit/3c6ad114d4dccaf99385e939d630ceb7eab7da1e
Assignee: nobody → ianb
Attachment #8912823 - Flags: review?(jhirsch) → review?(kmaglione+bmo)
kmag: thoughts on the review process for fixes like these?
Comment on attachment 8912823 [details]
Bug 1403661 - stop Screenshots in Private Browsing;

https://reviewboard.mozilla.org/r/184128/#review189918
Attachment #8912823 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/42f0bbe5be03
stop Screenshots in Private Browsing; r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42f0bbe5be03
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(In reply to Ian Bicking (:ianbicking) from comment #2)
> kmag: thoughts on the review process for fixes like these?

I'm happy with the current process for the moment, but please let me know if it isn't working for you, for whatever reason.

Also, feel free to ping me if I'm not getting to things fast enough.
Comment on attachment 8912823 [details]
Bug 1403661 - stop Screenshots in Private Browsing;

Approval Request Comment
[Feature/Bug causing the regression]:
In past versions of Screenshots we disabled Screenshots in Private Browsing mode, due to some bugs with respecting the isolated context. In the change to Photon pageActions this regressed. This patch reintroduces blocking Screenshots via pageAction.

[User impact if declined]:
Users may use Screenshots in Private Browsing mode, and could be uncomfortable or surprised by downloaded shots showing up in the main download manager, and uploaded shots being associated with their main Screenshots account.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
ni'ing Cosmin

[Needs manual test from QE? If yes, steps to reproduce]: 
1. Open a Private Browsing window
2. Open a page in that window
3. Via the "..." menu start Screenshots
4. Correct behavior is that a popup error message will come up, that informs the user that Screenshots can't be used in Private Browsing Mode. Note that error popups are blocked during the first ~20s of a Firefox session.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Very small change that makes the tab objects consistent regardless of how Screenshots is started

[String changes made/needed]:
None
Attachment #8912823 - Flags: approval-mozilla-beta?
Flags: needinfo?(cosmin.muntean)
I have tested this on latest Nightly (58.0a1, Build ID: 20171005220204)(x64 and x32) and the Screenshots no longer works on Private Windows. When you try to use it in Private Window the "Screenshots is disabled in Private Browsing Mode" warning is correctly displayed. During testing I haven't found any new issues.

I can confirm that Screenshots could be activated in Private Window if it was used from "Page Action" menu on older Nightly builds eg: build from 2017-10-02).

Verified as fixed on Windows 10 x64, Windows 7 x64, Mac OS 10.12 and Ubuntu 14.04 x64 using latest Nightly (58.0a1) build.
Flags: needinfo?(cosmin.muntean)
Comment on attachment 8912823 [details]
Bug 1403661 - stop Screenshots in Private Browsing;

Severe recent regression, beta57+
Attachment #8912823 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reproduced the initial issue on 58.0a1 (2017-09-27).
57.0b7 build1 (20171009192146) and 58.0a1 (2017-10-10) builds are displaying the informative popup error message in private browsing, but not from the first try (used a clean profile) (on Windows 10 x64 and Ubuntu 16.04 x64 the message is displayed from the second try, on Mac 10.11.6 the message is displayed from the third try). Also, when the page action menu is opened for the first time, the "Take a Screenshot" option is displayed and using it nothing happens. When the page menu is opened for the second time, the "Firefox Screenshots" option is displayed instead. 
Any thoughts about this?
Flags: needinfo?(ianb)
Forgot to provide a screencast for the behavior described above. Here it is https://goo.gl/niAVS2.
Because we were worried about our prominent error popups annoying users (especially if they aren't trying to interact with Screenshots), we have a timer on all error messages (including this one). I thought the timer was from the start of the Screenshots extension, but I just looked at the code again and realized the timer actually starts when you first *activate* Screenshots (because we're lazily loading that code).

So I expect that the behavior right now is: if you start Firefox, open a Private Browsing Window, and activate Screenshots, then the error message will not be displayed. If you use Screenshots on a normal window, then later use it in Private Browsing, the error message should be displayed properly.

I've opened this issue about the problem: https://github.com/mozilla-services/screenshots/issues/3707
Flags: needinfo?(ianb)
Joni: FYI for you, no immediate action for SUMO but you are welcome to check with Ian to see what he recommends so we are prepared to support Screen Shots users in 57. Thank you.
Flags: needinfo?(jsavage)
We discussed this in our meeting, and we don't think we should do anything additional for 57. I don't think it will need any support – if the user is confused they are most likely to just try again, and on the second time they'll get the error message. In 58 we are planning to allow Screenshots in Private Browsing windows (download-only), so the bug will be moot.
Based on comment 11, comment 13 and comment 15, I will mark the 57 build as verified fixed.
Status: RESOLVED → VERIFIED
Thanks for looping us in. It looks like no action is needed on our part.
Flags: needinfo?(jsavage)
You need to log in before you can comment on or make changes to this bug.