If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

about:support - use the new in-content styling

RESOLVED FIXED in Firefox 41



2 years ago
2 years ago


(Reporter: Paenglab, Assigned: Paenglab)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)



(1 attachment, 3 obsolete attachments)



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

Comment 1

2 years ago
Created attachment 8601023 [details] [diff] [review]

Simply import in-content/common.css and tweak some colors and margins/paddings.
Assignee: nobody → richard.marti
Attachment #8601023 - Flags: review?(MattN+bmo)
Comment on attachment 8601023 [details] [diff] [review]

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.
Attachment #8601023 - Flags: review?(MattN+bmo) → review-

Comment 3

2 years ago
Created attachment 8601625 [details] [diff] [review]

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.
Attachment #8601023 - Attachment is obsolete: true
Attachment #8601625 - Flags: review?(MattN+bmo)

Comment 4

2 years ago
Created attachment 8602250 [details] [diff] [review]

Updated to tip.
Attachment #8601625 - Attachment is obsolete: true
Attachment #8601625 - Flags: review?(MattN+bmo)
Attachment #8602250 - Flags: review?(MattN+bmo)
Comment on attachment 8602250 [details] [diff] [review]

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?
Attachment #8602250 - Flags: review?(MattN+bmo) → review+

Comment 6

2 years ago
Created attachment 8603717 [details] [diff] [review]

Now with more descriptive variable names.
Attachment #8602250 - Attachment is obsolete: true
Attachment #8603717 - Flags: review?(MattN+bmo)

Comment 7

2 years ago
Forgot to mention this needs bug 1162896 applied first which has checkin-needed.
Comment on attachment 8603717 [details] [diff] [review]

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

I apologize for the delay. This week has been hectic.
Attachment #8603717 - Flags: review?(MattN+bmo) → review+

Comment 9

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

The failures don't looking correlated to this patch.
Keywords: checkin-needed
Had to fix the bug number in the commit message. Please double-check that in the future.

Comment 11

2 years ago
Keywords: checkin-needed


2 years ago
Blocks: 1097111
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41


2 years ago
Depends on: 1166855
You need to log in before you can comment on or make changes to this bug.