1125636 - Update about:config to use the new Project Chameleon style

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Comment 1

[Richard Marti (:Paenglab)]

Attachment: aboutConfigNewStyle.patch

Comment 2

[Richard Marti (:Paenglab)]

Attachment: aboutConfigNewStyle.patch

One change slipped out of the patch. Now it should be complete.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8557583 [details] [diff] [review]
aboutConfigNewStyle.patch

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

Beautiful :)

I think the page would fit in better if it used the background color common.css provides, since it's more of an application than a message page (keeping the warning part of this as you've done it, with the lighter background). ie:
* What this patch currently does: http://d.pr/i/1eW6G
* Using the slightly darker background: http://d.pr/i/15zEf
It's a minor thought - I'm happy for this to land as-is. Or you could just tweak it as a followup bug.

I think it'd be worth filing a bug to transition other code from warning.png/warning-16.png/warning-24.png/etc over to warning.svg

::: toolkit/components/viewconfig/content/config.xul
@@ +3,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<?xml-stylesheet href="chrome://global/skin/in-content/common.css" type="text/css"?>

Don't need to import this - info-pages.css already does it for you.

Comment 4

[Tim Nguyen :ntim]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #3)
> Comment on attachment 8557583 [details] [diff] [review]
> aboutConfigNewStyle.patch
> 
> Review of attachment 8557583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Beautiful :)
> 
> I think the page would fit in better if it used the background color
> common.css provides, since it's more of an application than a message page
> (keeping the warning part of this as you've done it, with the lighter
> background). ie:
> * What this patch currently does: http://d.pr/i/1eW6G
> * Using the slightly darker background: http://d.pr/i/15zEf
> It's a minor thought - I'm happy for this to land as-is. Or you could just
> tweak it as a followup bug.
Nice ! I haven't tested this yet, but I would probably change the font as well. (message-box)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Tim Nguyen [:ntim] from comment #4)
> I would probably change the font as well. (message-box)

Hah, yep - this was just mentioned on IRC. Should be sans-serif. I assume that's no longer being picked up in the tree because the stylesheet is no longer importing chrome://global/skin/. Trees are a bit odd, we'll probably need to specify the font in a rule that specifically targets the tree.

Comment 6

[Richard Marti (:Paenglab)]

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #5)
> (In reply to Tim Nguyen [:ntim] from comment #4)
> > I would probably change the font as well. (message-box)
> 
> Hah, yep - this was just mentioned on IRC. Should be sans-serif. I assume
> that's no longer being picked up in the tree because the stylesheet is no
> longer importing chrome://global/skin/. Trees are a bit odd, we'll probably
> need to specify the font in a rule that specifically targets the tree.

I've used the same font as the other about pages are using through: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#9

Also without my patch the tree is using message-box. Blair, could you check what font is used on your system in configTree without my patch?

Comment 7

[Tim Nguyen :ntim]

(In reply to Richard Marti (:Paenglab) from comment #6)
> (In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo?
> me!) from comment #5)
> > (In reply to Tim Nguyen [:ntim] from comment #4)
> > > I would probably change the font as well. (message-box)
> > 
> > Hah, yep - this was just mentioned on IRC. Should be sans-serif. I assume
> > that's no longer being picked up in the tree because the stylesheet is no
> > longer importing chrome://global/skin/. Trees are a bit odd, we'll probably
> > need to specify the font in a rule that specifically targets the tree.
> 
> I've used the same font as the other about pages are using through:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css#9
> 
> Also without my patch the tree is using message-box. Blair, could you check
> what font is used on your system in configTree without my patch?

The problem is that there's no page element. There's a window element though. One simple fix would be to add xul|window after xul|page and html|body in https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#9

Comment 8

[Richard Marti (:Paenglab)]

Attachment: aboutConfigNewStyle.patch

I've added xul|window to common.css to get the background color and the font. On #warningScreen I removed the now obsolete rules which are now coming from common.css.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8559257 [details] [diff] [review]
aboutConfigNewStyle.patch

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

Looks good :)

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8559257 [details] [diff] [review]
aboutConfigNewStyle.patch

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

::: toolkit/components/viewconfig/content/config.xul
@@ +3,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +<?xml-stylesheet href="chrome://global/skin/in-content/common.css" type="text/css"?>

Er, though as I mentioned in comment 3, you don't need to import common.css here - info-pages.css already imports it for you.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: aboutConfigNewStyle.patch

Addressed the review comment.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/dbe74171ca49

Comment 13

[Richard Marti (:Paenglab)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46b7008d4346

Where are failures in testEditorAdded@chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js:24 but I don't think my change for about:config has some influence to the developer tools.

Comment 14

[Carsten Book [:Tomcat]]

backed out for causing test bustages in dt tests like https://treeherder.mozilla.org/logviewer.html#?job_id=1951845&repo=fx-team

Comment 15

[Richard Marti (:Paenglab)]

Patrick or Heather, please could you look at this test failure and maybe provide a fix for the test? Blame has shown you has touched this test files.

I'm not so experienced in JS, but it looks like this test fails because I added a second style sheet to config.xul. I had run a try with only removing the new style sheet line in config.xul and let all other changes as they are and the test passed.

Thanks in advance.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Richard Marti (:Paenglab) from comment #15)
> Patrick or Heather, please could you look at this test failure and maybe
> provide a fix for the test? Blame has shown you has touched this test files.
> 
> I'm not so experienced in JS, but it looks like this test fails because I
> added a second style sheet to config.xul. I had run a try with only removing
> the new style sheet line in config.xul and let all other changes as they are
> and the test passed.
> 
> Thanks in advance.
This test relies on the number of stylesheets in about:config unfortunately. I don't think it should. I'll rewrite the test a little bit so it passes again with your patch and doesn't depend on about:config's internals.

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug1125636-make-test-browser_styleeditor_xul.js-not-depend-on-aboutconfig.patch

Hey Joe, could you review this simple styleeditor test change?
In fact, I have left the test logic unchanged, but made it use a new test XUL document instead of relying on about:config.

Comment 18

[Richard Marti (:Paenglab)]

Patrick, thanks a lot for looking into this!

Comment 19

[Richard Marti (:Paenglab)]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2624eb9fdd

Comment 20

[Richard Marti (:Paenglab)]

Tests are now all green :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/96ac8d46bf2a
https://hg.mozilla.org/integration/fx-team/rev/248a8d2e4a27

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96ac8d46bf2a
https://hg.mozilla.org/mozilla-central/rev/248a8d2e4a27

Comment 23

[Willy_ Foo_Foo]

I think this bug is causing

https://bugzilla.mozilla.org/show_bug.cgi?id=1132241

this bug

Comment 24

[Richard Marti (:Paenglab)]

(In reply to ElevenReds from comment #23)
> I think this bug is causing
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1132241
> 
> this bug

But this will be today the first time in nightly. So how could it affect this in yesterdays build?

Comment 25

[rsx11m]

This bug changed Toolkit and thus affects other applications like Thunderbird using the global themes as well. Please file those bugs actually as Toolkit so that they can be searched for more easily. Thanks.