Update about:config to use the new Project Chameleon style

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ntim, Assigned: Paenglab)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify -

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Posted patch aboutConfigNewStyle.patch (obsolete) — Splinter Review
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8557539 - Flags: review?(jaws)
Assignee

Comment 2

4 years ago
Posted 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+
Reporter

Comment 4

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

Comment 6

4 years ago
(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?
Reporter

Comment 7

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

Updated

4 years ago
Flags: firefox-backlog?
Assignee

Comment 8

4 years ago
Posted 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.
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 11

4 years ago
Addressed the review comment.
Attachment #8559257 - Attachment is obsolete: true
Attachment #8561940 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 13

4 years ago
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
Assignee

Comment 15

4 years ago
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)
Assignee

Comment 18

4 years ago
Patrick, thanks a lot for looking into this!
Attachment #8562657 - Flags: review?(jwalker) → review+
Assignee

Comment 20

4 years ago
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
Last Resolved: 4 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+

Comment 23

4 years ago
I think this bug is causing

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

this bug
Assignee

Comment 24

4 years ago
(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?

Updated

4 years ago
Depends on: 1132950
Reporter

Updated

4 years ago
Depends on: 1133002

Comment 25

4 years ago
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 → ---

Updated

4 years ago
Target Milestone: --- → mozilla38
Flags: qe-verify-
Reporter

Updated

4 years ago
Flags: in-testsuite+ → in-testsuite-
You need to log in before you can comment on or make changes to this bug.