985114 - Make use of CSS variables

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I just read that gecko 29 now has CSS variables, see here: http://mcc.id.au/blog/2013/12/variables

We should use these in Lightning and define a color theme through these variables.

Comment 1

[Stefan Sitter [:ssitter]]

This feature is controlled by a preference and not yet enabled by default. It might be enabled by default in Firefox 31 (Bug 957833). But as long as the user can disable the feature by flipping the preference I would not use it in Lightning.

Comment 2

[pupunmajumder]

I am new to open source and only have an academic background in development(Java,javascriythonpt,php,p some C). I was hoping for some guidance on getting starting. Thanks.

Comment 3

[amanjain110893]

I want to work on this bug can someone please assign this to me

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Stefan brings up a valid point in comment 1. For me personally it would be enough that the pref is enabled, but we need to at least make sure its enabled in a release build before we create the patch. To figure this out we need to wait for the next release before we can work on this bug.

I suggest to wait until May before working on this bug, and aside from that only one of you can do so. pupunmajumder asked first.

Comment 5

[Sireesha]

Hi, can I work on this bug?

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Stefan, afaik Thunderbird is already making use of CSS variables. Given Thunderbird would be broken if this pref is disabled, do you still think we should wait? The only other requirement would be to make sure that Seamonkey also has this enabled and working.


sireesha, please see comment 4. If Neither amanjain nor pupunmajumder is going to work on this bug, you are of course free to work on it.

For all of you, the first step would be to determine if a recent Seamonkey release has this enabled an working.

Comment 7

[Richard Marti (:Paenglab)]

The variables are enabled by default with Gecko 31 (see bug 957833). So it should be safe now to use them.

Comment 8

[Shekhar Prasad Rajak]

Can I work on this ???

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sure, go ahead. Docs on how to get started in the other bug you asked for. Do you want to do both at once or just one of them?

Comment 10

[Shekhar Prasad Rajak]

yeah trying both ..

Comment 11

[Richard Marti (:Paenglab)]

Shekhar, are you still working on this bug?

Comment 12

[Richard Marti (:Paenglab)]

Attachment: cssVariables.patch

No activity and feedback from Shekhar. I'm taking the bug.

Philipp, if you have better names for the variables, I can use them for the next patch.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8541486 [details] [diff] [review]
cssVariables.patch

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

Most of these comments should be seen as suggestions up for discussion, not review comments. While this is a great start and I would r+ this aside from nits, I think it would be a good idea to have the variable names describe the function rather than the technical xul thingy they are used in. This would mean less use of words like "daybox". Terms like "today", "weekend", "otherMonth" are already very good.

But maybe I am thinking too much into the SCSS world, where we could define a small set of 3-4 colors and then use functions like lighten, darken and rgba($color, .05) to manipulate the colors.

At a minimum, please clear the nits and the things I didn't mark with "extra".

::: calendar/base/themes/common/calendar-views.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/. */
> + 
> +window {

nit: There is an extra whitespace above this line. The same above the systemcolors rule.

@@ +4,5 @@
> + 
> +window {
> +    --fgColor: #000;
> +    --bgColor: #fff;
> +    --borderColor: #d2d2d2;

maybe viewFGColor / viewBGColor / viewBorder instead? I know the file is called calendar-views.css, but it sounds very generic right now. What if another file that also applies to the window wants to use this name?

On the other hand, prefixing everything with view seems redundant too. What do you think?

(Extra)

@@ +30,5 @@
> +    --monthDayOffLabelBGColor: #eaf7ca;
> +    --offTodayBGColor: #d7e8f8;
> +    --offSelectedBGColor: #f2edb2;
> +    --offtimeBGColor: #f0f0f0;
> +    --otherBoxColor: #6a6969;

Was this other month? Without checking the code I don't quite remember what it is. If I am right, then otherMonthBoxColor sounds good.

How does this compare to monthOtherBGColor and monthDayOtherBGColor?

@@ +34,5 @@
> +    --otherBoxColor: #6a6969;
> +    --dayLabelSelectedFGColor: #000;
> +    --dayLabelSelectedBGColor: #fffabc;
> +    --dragboxFGColor: -moz-dialogtext;
> +    --dragboxBGTexture: linear-gradient(#fe4b22, #feb822);

Maybe this variable should not just define the bg texture, but the background itself. You could change the css property from background-image to background and then just call this dragboxBackground (applies as well to the other *texture variables).

Extra: It might also be worth checking for the other properties and instead of naming them e.g. offTodayBGColor and offTodayFGColor, name them offTodayBackground and offTodayColor. What do you think?

@@ +57,5 @@
> +    --dayBoxOffSelectedBGColor: ThreeDLightShadow;
> +    --monthDayBoxSelectedFGColor: HighlightText;
> +    --monthDayBoxSelectedBGColor: Highlight;
> +    --monthDayBoxLabelFGColor: WindowText;
> +    --monthDayBoxLabelBGTexture: linear-gradient(rgba(0, 0, 0, .05), rgba(0, 0, 0, .05));

Given you take my above comment about using background instead of texture into account, you could just change this to rgba(0,0,0,.05) or #f2f2f2, or whatever predefined color may match.

Same goes for the other linear-gradient property and you might also want to translate the other rgba() colors.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: cssVariables.patch

(In reply to Philipp Kewisch [:Fallen] from comment #13)
> Comment on attachment 8541486 [details] [diff] [review]
> cssVariables.patch
> 
> Review of attachment 8541486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Most of these comments should be seen as suggestions up for discussion, not
> review comments. While this is a great start and I would r+ this aside from
> nits, I think it would be a good idea to have the variable names describe
> the function rather than the technical xul thingy they are used in. This
> would mean less use of words like "daybox". Terms like "today", "weekend",
> "otherMonth" are already very good.
> 
> But maybe I am thinking too much into the SCSS world, where we could define
> a small set of 3-4 colors and then use functions like lighten, darken and
> rgba($color, .05) to manipulate the colors.

I tried already to minimize the variables but the same color in normal theme could in [systemcolor] theme two or three colors depending where it is positioned to other elements because of the limited system colors.

> At a minimum, please clear the nits and the things I didn't mark with
> "extra".
> 
> ::: calendar/base/themes/common/calendar-views.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/. */
> > + 
> > +window {
> 
> nit: There is an extra whitespace above this line. The same above the
> systemcolors rule.

Fixed on all files.

> @@ +4,5 @@
> > + 
> > +window {
> > +    --fgColor: #000;
> > +    --bgColor: #fff;
> > +    --borderColor: #d2d2d2;
> 
> maybe viewFGColor / viewBGColor / viewBorder instead? I know the file is
> called calendar-views.css, but it sounds very generic right now. What if
> another file that also applies to the window wants to use this name?
> 
> On the other hand, prefixing everything with view seems redundant too. What
> do you think?

This is a good proposal. I used --view, --mm and --event to not overwriting each other and to be save for other variables which could be used in other parts of TB.

> (Extra)
> 
> @@ +30,5 @@
> > +    --monthDayOffLabelBGColor: #eaf7ca;
> > +    --offTodayBGColor: #d7e8f8;
> > +    --offSelectedBGColor: #f2edb2;
> > +    --offtimeBGColor: #f0f0f0;
> > +    --otherBoxColor: #6a6969;
> 
> Was this other month? Without checking the code I don't quite remember what
> it is. If I am right, then otherMonthBoxColor sounds good.
> 
> How does this compare to monthOtherBGColor and monthDayOtherBGColor?

Nothing, I'm using now --viewTimeBoxColor

> @@ +34,5 @@
> > +    --otherBoxColor: #6a6969;
> > +    --dayLabelSelectedFGColor: #000;
> > +    --dayLabelSelectedBGColor: #fffabc;
> > +    --dragboxFGColor: -moz-dialogtext;
> > +    --dragboxBGTexture: linear-gradient(#fe4b22, #feb822);
> 
> Maybe this variable should not just define the bg texture, but the
> background itself. You could change the css property from background-image
> to background and then just call this dragboxBackground (applies as well to
> the other *texture variables).

The --viewDragboxTexture is a gradient and using only one color would make it flat. 

> Extra: It might also be worth checking for the other properties and instead
> of naming them e.g. offTodayBGColor and offTodayFGColor, name them
> offTodayBackground and offTodayColor. What do you think?
> 
> @@ +57,5 @@
> > +    --dayBoxOffSelectedBGColor: ThreeDLightShadow;
> > +    --monthDayBoxSelectedFGColor: HighlightText;
> > +    --monthDayBoxSelectedBGColor: Highlight;
> > +    --monthDayBoxLabelFGColor: WindowText;
> > +    --monthDayBoxLabelBGTexture: linear-gradient(rgba(0, 0, 0, .05), rgba(0, 0, 0, .05));
> 
> Given you take my above comment about using background instead of texture
> into account, you could just change this to rgba(0,0,0,.05) or #f2f2f2, or
> whatever predefined color may match.
> 
> Same goes for the other linear-gradient property and you might also want to
> translate the other rgba() colors.

The --viewMonthDayBoxLabelTexture is used in [systemcolors] to darken different background colors of boxes in different states.

I used only rgba() where I needed some transparency to darken some system colors.

I had to add the -mm variables also to calendar-event-dialog.css to apply correctly to the minimonth in event-dialog.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8542170 [details] [diff] [review]
cssVariables.patch

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

(In reply to Richard Marti (:Paenglab) from comment #14)
> 
> > @@ +34,5 @@
> > > +    --otherBoxColor: #6a6969;
> > > +    --dayLabelSelectedFGColor: #000;
> > > +    --dayLabelSelectedBGColor: #fffabc;
> > > +    --dragboxFGColor: -moz-dialogtext;
> > > +    --dragboxBGTexture: linear-gradient(#fe4b22, #feb822);
> > 
> > Maybe this variable should not just define the bg texture, but the
> > background itself. You could change the css property from background-image
> > to background and then just call this dragboxBackground (applies as well to
> > the other *texture variables).
> 
> The --viewDragboxTexture is a gradient and using only one color would make
> it flat. 

A sorry I was unclear. I didn't mean to use a flat color everywhere, but to change the variable to be able to determine the whole background. Example:

Change the css to replace background-image with background:

fgdragbox[dragging="true"] {
  ...
  background: var(--dragboxBackground)
  ...
}


then use the following variables:

window {
  --dragboxBackground: linear-gradient(#fe4b22, #feb822);
}

window[systemcolors]
  --dragboxBackground: Highlight
}



> I had to add the -mm variables also to calendar-event-dialog.css to apply
> correctly to the minimonth in event-dialog.
So we have duplicate variable definitions? Maybe you can change the rule in minimonth.css like so:

window, dialog {
 --mm...: ...
}

Comment 16

[Richard Marti (:Paenglab)]

Attachment: cssVariables.patch

(In reply to Philipp Kewisch [:Fallen] from comment #15)
> Comment on attachment 8542170 [details] [diff] [review]
> cssVariables.patch
> 
> Review of attachment 8542170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Richard Marti (:Paenglab) from comment #14)
> 
> A sorry I was unclear. I didn't mean to use a flat color everywhere, but to
> change the variable to be able to determine the whole background.

Ah yes, for the dragbox this works -> changed. But the other texture needs a background-color and a background-image to darken the background color.

> > I had to add the -mm variables also to calendar-event-dialog.css to apply
> > correctly to the minimonth in event-dialog.
> So we have duplicate variable definitions? Maybe you can change the rule in
> minimonth.css like so:
> 
> window, dialog {
>  --mm...: ...
> }

I made this in my first patch and haven't tested the minimonth in dialog (shame on me). This doesn't work. I think this is because minimonth.css is loaded on startup with the tab and the dialog variables don't apply because there is no dialog and are deleted. But when the dialog is opened, the minimonth.css is reused from memory without the dialog rules. Maybe this is a specialty of the variables because the actual version with dialog[systemcolors] is working.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Richard Marti (:Paenglab) from comment #16)
> Created attachment 8542220 [details] [diff] [review]
> cssVariables.patch
> 
> (In reply to Philipp Kewisch [:Fallen] from comment #15)
> > Comment on attachment 8542170 [details] [diff] [review]
> > cssVariables.patch
> > 
> > Review of attachment 8542170 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Richard Marti (:Paenglab) from comment #14)
> > 
> > A sorry I was unclear. I didn't mean to use a flat color everywhere, but to
> > change the variable to be able to determine the whole background.
> 
> Ah yes, for the dragbox this works -> changed. But the other texture needs a
> background-color and a background-image to darken the background color.
Thats...tricky :-P Ok, lets keep it that way.


> 
> > > I had to add the -mm variables also to calendar-event-dialog.css to apply
> > > correctly to the minimonth in event-dialog.
> > So we have duplicate variable definitions? Maybe you can change the rule in
> > minimonth.css like so:
> > 
> > window, dialog {
> >  --mm...: ...
> > }
> 
> I made this in my first patch and haven't tested the minimonth in dialog
> (shame on me). This doesn't work. I think this is because minimonth.css is
> loaded on startup with the tab and the dialog variables don't apply because
> there is no dialog and are deleted. But when the dialog is opened, the
> minimonth.css is reused from memory without the dialog rules. Maybe this is
> a specialty of the variables because the actual version with
> dialog[systemcolors] is working.
This sounds like a bug in the variables implementation? Maybe we should file a core bug. Just for testing, does it work if you duplicate the whole rule, i.e.

dialog {
  ... all vars ...
}
window {
  ... all vars ...
}

This would tell us if one rule is being dropped or if its something else. You could also try using the :root pseudo-selector, i.e.


:root {
  ... all vars ...
}

Comment 18

[Richard Marti (:Paenglab)]

(In reply to Philipp Kewisch [:Fallen] from comment #17)

> This sounds like a bug in the variables implementation? Maybe we should file
> a core bug. Just for testing, does it work if you duplicate the whole rule,
> i.e.
> 
> dialog {
>   ... all vars ...
> }
> window {
>   ... all vars ...
> }
> 
> This would tell us if one rule is being dropped or if its something else.
> You could also try using the :root pseudo-selector, i.e.
> 
> 
> :root {
>   ... all vars ...
> }

Tried all this but it didn't work :(

Comment 19

[Richard Marti (:Paenglab)]

Attachment: cssVariables.patch

This should be the final patch. Philipp, thank you for your help, digging the minimonth issue.
I moved minimonth.css to shared. OS X doesn't use [systemcolors] but the other color variables are also in shared file and are needing only one place for definition.
With :root[systemcolors] minimonth {variables} and :root[systemcolors] minimonth {variables} only in minimonth.css it's working now in main window and in event dialog.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8543490 [details] [diff] [review]
cssVariables.patch

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

Looks good, r=philipp

Thanks for the patience!

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/comm-central/rev/e0d2a43ab539