Closed Bug 1125636 Opened 5 years ago Closed 5 years ago

Update about:config to use the new Project Chameleon style

Categories

(Toolkit :: Themes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: ntim, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
Attached patch aboutConfigNewStyle.patch (obsolete) — Splinter Review
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8557539 - Flags: review?(jaws)
Attachment #8557539 - Flags: review?(jaws) → review?(bmcbride)
Attached patch aboutConfigNewStyle.patch (obsolete) — Splinter Review
One change slipped out of the patch. Now it should be complete.
Attachment #8557539 - Attachment is obsolete: true
Attachment #8557539 - Flags: review?(bmcbride)
Attachment #8557583 - Flags: review?(bmcbride)
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.
Attachment #8557583 - Flags: review?(bmcbride) → review+
(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)
(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.
(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?
(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
Flags: firefox-backlog?
Attached patch aboutConfigNewStyle.patch (obsolete) — Splinter Review
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.
Attachment #8557583 - Attachment is obsolete: true
Attachment #8559257 - Flags: review?(bmcbride)
Comment on attachment 8559257 [details] [diff] [review]
aboutConfigNewStyle.patch

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

Looks good :)
Attachment #8559257 - Flags: review?(bmcbride) → review+
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.
Keywords: checkin-needed
Keywords: checkin-needed
Addressed the review comment.
Attachment #8559257 - Attachment is obsolete: true
Attachment #8561940 - Flags: review+
Keywords: checkin-needed
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.
backed out for causing test bustages in dt tests like https://treeherder.mozilla.org/logviewer.html#?job_id=1951845&repo=fx-team
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.
Flags: needinfo?(pbrosset)
Flags: needinfo?(fayearthur)
(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.
Flags: needinfo?(pbrosset)
Flags: needinfo?(fayearthur)
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.
Attachment #8562657 - Flags: review?(jwalker)
Patrick, thanks a lot for looking into this!
Attachment #8562657 - Flags: review?(jwalker) → review+
Tests are now all green :)
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/96ac8d46bf2a
https://hg.mozilla.org/mozilla-central/rev/248a8d2e4a27
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: firefox-backlog? → firefox-backlog+
I think this bug is causing

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

this bug
(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?
Depends on: 1132950
Depends on: 1133002
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.
Component: Theme → Themes
Product: Firefox → Toolkit
Target Milestone: Firefox 38 → ---
Target Milestone: --- → mozilla38
Flags: qe-verify-
Flags: in-testsuite+ → in-testsuite-
You need to log in before you can comment on or make changes to this bug.