1161156 - about:support - use the new in-content styling

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Richard Marti (:Paenglab)]

To be consistent on all in-content pages also about:support should use this styling.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Simply import in-content/common.css and tweak some colors and margins/paddings.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8601023 [details] [diff] [review]
aboutSupport.patch

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

r- for the magic numbers (color codes).

::: toolkit/themes/shared/aboutSupport.css
@@ +1,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/. */
>  
> +@import url("chrome://global/skin/in-content/common.css");

My understanding is that it's more performant to add this as a new <link> in aboutSupport.xhtml so parsing of this file doesn't have to start before loading common.css in parallel.

@@ +7,2 @@
>  html {
> +  background-color: #fbfbfb;

If we have a style guide (e.g. Chameleon), why are we using magic numbers (color codes) instead of giving names to the colours like the system colors do? The CSS variable names for the colours can be defined in common.inc.css and referred to here.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

On Sunday I also thought about variables and planned to do this later :). But now as you want it I implemented it on common.css and aboutSupport.css. Later I file bugs for the other consumers of common.css to use the variables.

I used for the variable names the ones are used here: http://people.mozilla.org/~jgruen/chameleon/old/#nav5

I added also variables which are actually not used, like the green or red, but who knows if they are used in the future.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Updated to tip.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8602250 [details] [diff] [review]
aboutSupport.patch

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

::: toolkit/themes/shared/in-content/common.inc.css
@@ +6,5 @@
>  @namespace html "http://www.w3.org/1999/xhtml";
>  @namespace xul "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
> +html|html,
> +xul|*:root {

*|*:root to replace the 2

@@ +11,5 @@
> +  --in-content-page-background: #fbfbfb;
> +  --in-content-light-text: #c1c1c1;
> +  --in-content-dark-text: #333333;
> +  --in-content-light-grey: #f2f2f2;
> +  --in-content-light-grey-hover: #ebebeb;

in-content-page-background --in-content-(light|dark)-text are decent semantic names whereas the others are simply describing the colour and the hover/active alternate version. Semantic naming like we have for the system colours is preferred since you can change the values without the name being wrong (e.g. suppose the stuff that's currently green should be orange in a redesign) and you shouldn't have to audit all uses for a change as developers should only be using colours that semantically make sense. e.g. nobody should use in-content-page-background for a text colour. Since the spec doesn't define most of these colours in that way (at least as far I can see), I leave it for you to do your best to use semantic names instead of descriptive names where possible.

@@ +116,5 @@
>  
>  xul|tabs {
>    margin-bottom: 15px;
> +  border-top: 1px solid var(--in-content-light-text);
> +  border-bottom: 1px solid var(--in-content-light-text);

Text colours for borders is interesting… Should the colour be forked to two names with the same value?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: aboutSupport.patch

Now with more descriptive variable names.

Comment 7

[Richard Marti (:Paenglab)]

Forgot to mention this needs bug 1162896 applied first which has checkin-needed.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8603717 [details] [diff] [review]
aboutSupport.patch

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

I apologize for the delay. This week has been hectic.

Comment 9

[Richard Marti (:Paenglab)]

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

The failures don't looking correlated to this patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Had to fix the bug number in the commit message. Please double-check that in the future.

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d7104ec78e7