Lift the lightweight theme restrictions for private windows

VERIFIED FIXED in Firefox 53

Status

()

defect
P2
major
VERIFIED FIXED
6 years ago
10 months ago

People

(Reporter: c.ascheberg, Assigned: bgrins)

Tracking

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 53+, firefox53 verified)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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

Comment 2

6 years ago
(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.
(Reporter)

Comment 3

6 years ago
(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.

Comment 4

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
Posted 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 7

6 years ago
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)
(Reporter)

Comment 8

6 years ago
Posted 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
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 852295

Comment 10

6 years ago
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)
(Reporter)

Comment 11

6 years ago
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 12

6 years ago
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+

Updated

6 years ago
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
(Reporter)

Updated

6 years ago
Duplicate of this bug: 860803
(Reporter)

Comment 14

6 years ago
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-
(Reporter)

Comment 16

6 years ago
(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

Comment 17

6 years ago
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

Updated

2 years ago
Blocks: 1320948

Updated

2 years ago
Blocks: 1331276
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
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 21

2 years ago
mozreview-review
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

Comment 23

2 years ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3f99c3904f1
Enable lightweight themes in private windows;r=dao

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3f99c3904f1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Comment 25

2 years ago
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]
Added to Fx53 Aurora release notes.
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+

Updated

10 months ago
Duplicate of this bug: 1458880
You need to log in before you can comment on or make changes to this bug.