Closed Bug 854126 Opened 7 years ago Closed 4 years ago

Lift the lightweight theme restrictions for private windows

Categories

(Firefox :: Private Browsing, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
relnote-firefox --- 53+
firefox53 --- verified

People

(Reporter: c.ascheberg, Assigned: bgrins)

References

()

Details

Attachments

(2 files, 3 obsolete files)

Personas styles are not shown
- in PB windows
- if Firefox runs in permanent private browsing mode

I think it should be possible to use personas in both cases.
I guess not showing personas in permanent PB mode really is a bug.

But is it indeed intended to not show them in private browsing windows although there is a new PB mask icon for the purpose of signaling PB windows?
Blocks: 749394
OS: Windows 7 → All
(In reply to comment #1)
> I guess not showing personas in permanent PB mode really is a bug.

Yes!

> But is it indeed intended to not show them in private browsing windows although
> there is a new PB mask icon for the purpose of signaling PB windows?

Well, it is only "intentional" in that the way that we have themes private windows for now is incompatible with theming it with a persona.  But there is no huge reason why that cannot be fixed in the future, but that will probably happen after the change to Australis.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Well, it is only "intentional" in that the way that we have themes private
> windows for now is incompatible with theming it with a persona.

Could you be more specific about the incompatibility?
I can see that personas and the new mask icon do not always blend in perfectly, but currently I do not see that this should be reason to not show personas at all. In fact, there is no design change for the default Win7 config. Or is there / will there be any?

Independently, if permanent PB mode does not cause such incompatibility, I can surely provide a patch for that case.
(In reply to comment #3)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > Well, it is only "intentional" in that the way that we have themes private
> > windows for now is incompatible with theming it with a persona.
> 
> Could you be more specific about the incompatibility?
> I can see that personas and the new mask icon do not always blend in perfectly,
> but currently I do not see that this should be reason to not show personas at
> all. In fact, there is no design change for the default Win7 config. Or is
> there / will there be any?

Oh, I meant incompatibility in the implementation.  Basically, we theme the private windows the same way that we apply a persona to a window, which means that we cannot use the persona infrastructure to private windows since they already have a "persona" applied to them (or at least a persona of some sort.)

> Independently, if permanent PB mode does not cause such incompatibility, I can
> surely provide a patch for that case.

Sure.  I _think_ all you need to do is to modify http://mxr.mozilla.org/mozilla-central/source/toolkit/content/LightweightThemeConsumer.jsm to also check for permanentPrivateBrowsingEnabled every time we check isWindowPrivate there, and basically treat all windows in permanent PB mode as non-private windows.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Oh, I meant incompatibility in the implementation.

Ah, I had a closer look at this. There seems to be no incompatibility on Windows (see attachment) because the personas is a background-image on the #main-window element, while the PB mask icon is set as background-image on the overlying #toolbar-menubar element.
But from your patch in bug 749394 it looks like this is not the case on Mac OS and I do not know if there is an easy similar solution.
Attached patch patch for permanent PB mode (obsolete) — Splinter Review
This patch only fixes the use of personas with permanent PB mode.
(In case there is no easy general fix for Mac OS. The use of personas in normal PB windows [as seen in the previously attached screenshot] is not possible with this patch.)
Assignee: nobody → c.ascheberg
Attachment #729057 - Flags: review?(ehsan)
Comment on attachment 729057 [details] [diff] [review]
patch for permanent PB mode

Review of attachment 729057 [details] [diff] [review]:
-----------------------------------------------------------------

You need to make a similar adjustment to the other isWindowPrivate call in this file, I think.  Also, please fix the indentation.

(And yeah, Windows is fine here, it's Mac (and I think Linux too) which have this problem.
Attachment #729057 - Flags: review?(ehsan)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #729057 - Attachment is obsolete: true
Attachment #731129 - Flags: review?(ehsan)
It appears that identical work has been done in 852295 and it's sitting waiting to be committed. Sorry about the duplication of effort.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 852295
Comment on attachment 731129 [details] [diff] [review]
patch v2

Hmm, yeah, Josh is right.  Sorry Christian, I had not seen that bug... :/
Attachment #731129 - Flags: review?(ehsan)
See comment 5.
This seems to work on Linux, too. (Linux design change was implemented in bug 827454.)
Attachment #731129 - Attachment is obsolete: true
Attachment #737663 - Flags: feedback?(ehsan)
Comment on attachment 737663 [details] [diff] [review]
enable personas on windows and linux

Review of attachment 737663 [details] [diff] [review]:
-----------------------------------------------------------------

I see.  This would be an improvement I think but before we can take this patch, you need to test on both tabs on top and tabs on bottom on Windows and Linux (and with both the classic and aero theme in the case of Windows).  If they all look fine, please ask Dao for the final review since he knows everything that there is to know about our themes.

Thanks!
Attachment #737663 - Flags: feedback?(ehsan) → feedback+
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: no personas in (permanent) private browsing mode → Lift the lightweight theme restrictions for private windows on Windows and Linux
Duplicate of this bug: 860803
Comment on attachment 737663 [details] [diff] [review]
enable personas on windows and linux

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> Comment on attachment 737663 [details] [diff] [review]
> enable personas on windows and linux
> 
> Review of attachment 737663 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see.  This would be an improvement I think but before we can take this
> patch, you need to test on both tabs on top and tabs on bottom on Windows
> and Linux (and with both the classic and aero theme in the case of Windows).

I did not detect any problems in those cases.
Attachment #737663 - Flags: review?(dao)
Comment on attachment 737663 [details] [diff] [review]
enable personas on windows and linux

There's nothing inherent in OS X that would prevent us from applying lightweight themes in private windows. It's just that our default theme doesn't like that, but we shouldn't mix theme with platform requirements like this patch does.

If we want to support lightweight themes in private windows, we should just do that across the board and make our default theme on OS X deal with that.
Attachment #737663 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #15)
> If we want to support lightweight themes in private windows, we should just
> do that across the board and make our default theme on OS X deal with that.

I do not know how to fix the OS X theme, partly because I do not have a Mac for testing.
Will the Australis theme fix this?
Assignee: c.ascheberg → nobody
Is there any hope that in the end we will again be able to see custom personas in REGULAR Private Browsing windows, or not? I am using FF 24.0 under Linux.

Or should we just rely on third party add-ons like https://addons.mozilla.org/fr/firefox/addon/private-browsing-personas/ ? It does what it promises, but I have actually no idea on which level it interferes with (disables?) the Private Browsing settings...
Summary: Lift the lightweight theme restrictions for private windows on Windows and Linux → Lift the lightweight theme restrictions for private windows
Priority: -- → P2
Hardware: x86_64 → All
Blocks: 1320948
Blocks: 1331276
Testing locally in OSX I don't see anything out of the ordinary with lightweight themes in PB windows.  Trying to get a custom mozscreenshots run that uses a private window to see screenshots across platforms / configurations.
Assignee: nobody → bgrinstead
Status: REOPENED → ASSIGNED
Comment on attachment 8827532 [details]
Bug 854126 - Enable lightweight themes in private windows;

https://reviewboard.mozilla.org/r/105186/#review106158
Attachment #8827532 - Flags: review?(dao+bmo) → review+
Attachment #737663 - Attachment is obsolete: true
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3f99c3904f1
Enable lightweight themes in private windows;r=dao
https://hg.mozilla.org/mozilla-central/rev/c3f99c3904f1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Release Note Request (optional, but appreciated)
[Why is this notable]: Lightweight themes didn't work inside private windows before
[Affects Firefox for Android]: Unsure. Dão, do you know?
[Suggested wording]: Lightweight themes are now applied in private browsing windows
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: needinfo?(dao+bmo)
(In reply to Brian Grinstead [:bgrins] from comment #25)
> [Affects Firefox for Android]: Unsure. Dão, do you know?

Doesn't affect Firefox for Android. It has its own LightweightThemeConsumer.jsm: https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/LightweightThemeConsumer.jsm
Flags: needinfo?(dao+bmo)
Successfully managed to reproduce this bug on Nightly 22.0a1 (2013-03-23) on KDE Neon (64 Bit)!

This issue is now Verified as Fixed on Latest Firefox Dev. Edition 53.0a2 (2017-01-24) (64-bit)

Build ID: 20170124181714
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
OS: Linux Linux 4.4.0-59-generic; KDE Neon (64 Bit)
QA Whiteboard: [bugday-20170125]
Flags: qe-verify+
Reproduced the issue on Nightly 22.0a1 (2013-03-23) on Windows 10 x64.
The issue is now VERIFIED FIXED using Windows 10 x64, Mac OS X 10.11.6 & Ubuntu 14.04 LTS x64 on Firefox Beta 53.0b1 (id: 20170307064827)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1458880
You need to log in before you can comment on or make changes to this bug.