1133380 - about:privatebrowsing doesn't display properly in Classic any more

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[[e-mail]]

Bug 1125636 changed the theming of about:config which we were borrowing from.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[rsx11m]

Does this break modern? We don't have chrome://global/skin/in-content/info-pages.css there...

Comment 3

[[e-mail]]

Missing CSS files get ignored, although they might generate warnings in the Error Console (but then they would for about:config anyway so that's a separate bug.)

Comment 4

[rsx11m]

You are correct, there is no warning in the Error Console for a missing CSS file when looking at about:config on current trunk. However, that bug has changed enough to break theming of the warning box for about:config, I've filed bug 1133582 on that issue.

Comment 5

[[e-mail]]

Bah, it's not that easy, because of bug 1133582 needing to change some of Modern's CSS.

Comment 6

[rsx11m]

Neil, let's take a step back for a moment. As was mentioned in today's status meeting, the new Toolkit style (obviously adopting some Windows 8.1 design principles) doesn't match the overall style of our themes well. Thus, the question is if we simply want to resolve the breakage and otherwise adopt that style (which personally I don't like either), or if we want to retain the old appearance and thus get independent from whatever Firefox decides the "correct" styles of those pages are.

This would obviously imply forking the respective files of the toolkit/ implementation for suite/ but might be an easy(-ier) solution for bug 1133582, thus making this issue here a WFM once config.xul and config.css are forked.

Comment 7

[Stefan [:stefanh]]

Comment on attachment 8564813 [details] [diff] [review]
Proposed patch

I haven't checked, but my guess is that http://mxr.mozilla.org/comm-central/source/suite/themes/classic/mac/communicator/aboutPrivateBrowsing.css needs some love too.

Comment 8

[Philip Chee]

Comment on attachment 8564813 [details] [diff] [review]
Proposed patch

1. It's still very "Australis" like. The <button> elements have -moz-appearance: none; (coming from .../in-content/common.css). For classic/default we should stick to the native widgets as much as possible.

2. The font size for normalTitle / privateTitle is 2.5em == 37.5px is overly large. I suspect that the Firefox UX people all have retina displays...

The Australis look is too different from the rest of our UI and is rather clashing. Perhaps something like about:neterror ?

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #8)
> The Australis look is too different from the rest of our UI and is rather
> clashing. Perhaps something like about:neterror ?

OK so I was confused by bug 1097111 which wants to change all of Firefox's about:pages; many of them are specific to Firefox so don't affect us; about:neterror appears to be in that category. However there are some that will affect us such as about:about, buildconfig, cache, crashes, license, rights.

Comment 10

[rsx11m]

Add about:support to the list, that's why I opened meta bug 1133743 to keep track of those.

Assuming that .../in-content/common.css will be used by all of those pages, would it be a better approach to override that file, given that it essentially overrides all/many xul.css definitions?

Comment 11

[[e-mail]]

We can, although it's a little hacky - the override applies to all themes and someone already complained about our existing override.

Comment 12

[Philip Chee]

Unfortunatly overrides make life difficult for third party theme authors and we don't want to do that.

Comment 13

[Philip Chee]

FYI In one of my extensions I used the style directive in chrome.manifest to style about:addons

Comment 14

[[e-mail]]

Attachment: Proposed patch

Just a minor tweak to play nicer with bug 1133582.

Comment 15

[Philip Chee]

Comment on attachment 8567306 [details] [diff] [review]
Proposed patch

I guess this is the best we can do for the moment. r=me

Comment 16

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #7)
> Comment on attachment 8564813 [details] [diff] [review]
> Proposed patch
> 
> I haven't checked, but my guess is that
> http://mxr.mozilla.org/comm-central/source/suite/themes/classic/mac/
> communicator/aboutPrivateBrowsing.css needs some love too.

Yeah, I was right - it needs to be changed.

Comment 17

[[e-mail]]

Attachment: With Mac changes

I also removed the now unused warningBoxIcon.

Comment 18

[Philip Chee]

Comment on attachment 8573509 [details] [diff] [review]
With Mac changes

r=me for the non-mac changes.

Comment 19

[Stefan [:stefanh]]

Comment on attachment 8573509 [details] [diff] [review]
With Mac changes

Thanks for doing this, Neil. r=me for the Mac changes.

Comment 20

[[e-mail]]

Pushed comm-central changeset c9452fadc613.

Comment 21

[[e-mail]]

Comment on attachment 8573509 [details] [diff] [review]
With Mac changes

[Approval Request Comment]
Regression caused by (bug #): 1125636
User impact if declined: Ugliness
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None

Comment 22

[[e-mail]]

Pushed comm-aurora changeset c42950dafe40.