Closed
Bug 1125636
Opened 10 years ago
Closed 10 years ago
Update about:config to use the new Project Chameleon style
Categories
(Toolkit :: Themes, defect)
Toolkit
Themes
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: ntim, Assigned: Paenglab)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Updated•10 years ago
|
Attachment #8557539 -
Flags: review?(jaws) → review?(bmcbride)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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•10 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)
Comment 5•10 years ago
|
||
(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•10 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•10 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•10 years ago
|
Flags: firefox-backlog?
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•10 years ago
|
||
Addressed the review comment.
Attachment #8559257 -
Attachment is obsolete: true
Attachment #8561940 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/dbe74171ca49
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 13•10 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.
Comment 14•10 years ago
|
||
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•10 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)
Comment 16•10 years ago
|
||
(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)
Comment 17•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8562657 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2624eb9fdd
Assignee | ||
Comment 20•10 years ago
|
||
Tests are now all green :)
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/96ac8d46bf2a https://hg.mozilla.org/integration/fx-team/rev/248a8d2e4a27
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/96ac8d46bf2a https://hg.mozilla.org/mozilla-central/rev/248a8d2e4a27
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Updated•10 years ago
|
Iteration: --- → 38.3 - 23 Feb
Flags: firefox-backlog? → firefox-backlog+
Comment 23•10 years ago
|
||
I think this bug is causing https://bugzilla.mozilla.org/show_bug.cgi?id=1132241 this bug
Assignee | ||
Comment 24•10 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?
Comment 25•10 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•10 years ago
|
Flags: qe-verify-
Reporter | ||
Updated•10 years ago
|
Flags: in-testsuite+ → in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•