Closed Bug 1504190 Opened 11 months ago Closed 8 months ago

Follow bug 1504187 to make more of the main UI dark

Categories

(Calendar :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(2 files, 8 obsolete files)

With bug 1504187 all trees will be black. Lightning without this patch will look weird.
Attached patch 1504190-themeCalendar.patch (obsolete) — Splinter Review
This patch makes almost the whole GUI themeable. The minimonth and the calendar view aren't themed as this doesn't work with the limited WE colours. But we can look into it in a follow-up bug.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9022117 - Flags: review?(makemyday)
To see how it looks you need bug 1504187 applied too.
Attached patch 1504190-themeCalendar.patch v2 (obsolete) — Splinter Review
The --chrome-color variable will be removed soon. I exchanged it with --lwt-text-color in this patch.
Attachment #9022117 - Attachment is obsolete: true
Attachment #9022117 - Flags: review?(makemyday)
Attachment #9022300 - Flags: review?(makemyday)
Comment on attachment 9022300 [details] [diff] [review]
1504190-themeCalendar.patch v2

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

Thanks for moving this forward. I have some findings when appying the patch (and the ones form bug 1504187) - all observed with Win10:

1. The parts of the todaypane with the default theme has areas with grey and white background color. With the dark theme, these areas are colored equally and not distinguishable.

2. The unifinder filter drowdown control in the calendar view has the expected grey color when being collapsed but a white when being expanded.

3. The lock icons for read-only calendars in the calendar list are now hard to distinguish from the background.

4. In calendar view, the selectors for the view mode are already adopted to the dark theme, but the selected view tab is still highlighted with a white background.

5. In task view, when selecting a task, the area holding the description still has a white background color (other then email content, this cannot be styled individually on item level, so it's probably straight forward to make it themable as well).

Codewise, just two comments

::: calendar/base/content/calendar-task-view.xul
@@ +31,5 @@
>                           tooltiptext="&calendar.newtask.button.tooltip;"
>                           observes="calendar_new_todo_command"/>
>            <textbox id="view-task-edit-field"
>                     flex="1"
> +                   class="task-edit-field searchBox themeableSearchBox"

The doesn't seem to have a definition of themeableSearchBox, so its probably definied globally, but the name indicates that it's a subset of searchBox. If so, doesn't it inherit searchBox's properties, so that you just need to reference only the themeableSearchBox here?

::: calendar/base/themes/common/today-pane.css
@@ +244,2 @@
>    background-color: #DFEAF4;
> +  color: #000000;

Shouldn't be used theme color variables here instead of hard coding? That would apply also to definitions above.
Attachment #9022300 - Flags: feedback+
(In reply to [:MakeMyDay] from comment #4)
> 3. The lock icons for read-only calendars in the calendar list are now hard
> to distinguish from the background.

The same applies for the warning triangle that is displayed in case of unavailable calendars
(In reply to [:MakeMyDay] from comment #4)
> 
> 2. The unifinder filter drowdown control in the calendar view has the
> expected grey color when being collapsed but a white when being expanded.

Please can you add a screenshot? I'm not sure where you see this.
Attached image darktheme-unifinder.png
Sure. If the unifinder isn't enabled in your calendar view, you can do so with the |Find Event| button in the toolbar.
Attached patch 1504190-themeCalendar.patch v3 (obsolete) — Splinter Review
(In reply to [:MakeMyDay] from comment #4)
> Comment on attachment 9022300 [details] [diff] [review]
> 1504190-themeCalendar.patch v2
> 
> Review of attachment 9022300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. The parts of the todaypane with the default theme has areas with grey and
> white background color. With the dark theme, these areas are colored equally
> and not distinguishable.

Changed this to use the same colour as the QFB toolbar (darker than the rest). The unifinder-searchBox and the task-addition-box have now the same background colour to separate from the toolbar above.

> 2. The unifinder filter drowdown control in the calendar view has the
> expected grey color when being collapsed but a white when being expanded.

This is expected as the menulist is active and open. Add the "Folder Location" menulist to the main toolbar and you see the same.

> 3. The lock icons for read-only calendars in the calendar list are now hard
> to distinguish from the background.

Fixed and we need less icons now. :-)

> 4. In calendar view, the selectors for the view mode are already adopted to
> the dark theme, but the selected view tab is still highlighted with a white
> background.

I made this to better fit with the white content below. In main window the tabs do also fit with the toolbar colour. But I can change when you still want this.

> 5. In task view, when selecting a task, the area holding the description
> still has a white background color (other then email content, this cannot be
> styled individually on item level, so it's probably straight forward to make
> it themable as well).
> 
> Codewise, just two comments
> 
> ::: calendar/base/content/calendar-task-view.xul
> @@ +31,5 @@
> >                           tooltiptext="&calendar.newtask.button.tooltip;"
> >                           observes="calendar_new_todo_command"/>
> >            <textbox id="view-task-edit-field"
> >                     flex="1"
> > +                   class="task-edit-field searchBox themeableSearchBox"
> 
> The doesn't seem to have a definition of themeableSearchBox, so its probably
> definied globally, but the name indicates that it's a subset of searchBox.
> If so, doesn't it inherit searchBox's properties, so that you just need to
> reference only the themeableSearchBox here?

Done. the changed searchBox.css are in this patch too.

> ::: calendar/base/themes/common/today-pane.css
> @@ +244,2 @@
> >    background-color: #DFEAF4;
> > +  color: #000000;
> 
> Shouldn't be used theme color variables here instead of hard coding? That
> would apply also to definitions above.

This items are already hard coded before. I had to add the text colour to not get the colour we set with the WE-theme.

If we want to change this to use systemColor too, we can do this in a new bug.
Attachment #9022300 - Attachment is obsolete: true
Attachment #9022300 - Flags: review?(makemyday)
Attachment #9022375 - Flags: review?(makemyday)
Attached patch 1504190-themeCalendar.patch v4 (obsolete) — Splinter Review
Updated to tip after changes from bug 1509194.
Attachment #9022375 - Attachment is obsolete: true
Attachment #9022375 - Flags: review?(makemyday)
Attachment #9027139 - Flags: review?(makemyday)
I'm noticing that the usage of theme variables is moving away from what it should be...

An example is using var(--sidebar-text-color) on top of var(--toolbar-bgcolor)... and quite a few others.

Here's how Firefox applies theme colors:
- Everything inside a sidebar: --sidebar-{background|text|border}-color
- Everything inside a toolbar (excluding sidebar toolbars): --toolbar-bgcolor/color (falls back to --lwt-text-color when not specified)
- Everything on the titlebar: --lwt-accent-color/--lwt-text-color

If TB wants to apply the sidebar properties on all trees is totally fine, but not following the background/text color variable pairs will likely cause WE themes to be inaccessible on Thunderbird. Notably because a text color for one area does not necessarily contrast with the background color of another area.
Flags: needinfo?(richard.marti)
Attached patch 1504190-themeCalendar.patch v5 (obsolete) — Splinter Review
The text colours should now correspond to the background colours.
Attachment #9027139 - Attachment is obsolete: true
Attachment #9027139 - Flags: review?(makemyday)
Flags: needinfo?(richard.marti)
Attachment #9028016 - Flags: review?(makemyday)
Comment on attachment 9028016 [details] [diff] [review]
1504190-themeCalendar.patch v5

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

::: calendar/base/themes/common/calendar-task-view.css
@@ +34,5 @@
> +    border-color: var(--sidebar-border-color, hsla(0,0%,60%,.4));
> +}
> +
> +:root[lwt-tree] #calendar-task-details-container {
> +    background-color: var(--toolbar-bgcolor);

Not setting color: var(--toolbar-color); will cause themes like:

https://addons.mozilla.org/en-US/firefox/addon/arc-darker/?src=search

to look broken.
It doesn't look broken. The text colour is defined in the same file on line 29.
(In reply to Richard Marti (:Paenglab) from comment #13)
> It doesn't look broken. The text colour is defined in the same file on line
> 29.

So, https://addons.mozilla.org/en-US/firefox/addon/arc-darker/ doesn't look broken because it doesn't trigger [lwt-tree] to turn on, but if it did turn on [lwt-tree], #calendar-task-details-container would have light gray as background color (--toolbar-bgcolor) and also light gray as text color (--lwt-text-color). This is why it's a good idea to set var(--toolbar-color) as color here.
Attached patch 1504190-themeCalendar.patch v6 (obsolete) — Splinter Review
Okay, this should do it.
Attachment #9028016 - Attachment is obsolete: true
Attachment #9028016 - Flags: review?(makemyday)
Attachment #9028424 - Flags: review?(makemyday)
(In reply to Richard Marti (:Paenglab) from comment #8)
> > 1. The parts of the todaypane with the default theme has areas with grey and
> > white background color. With the dark theme, these areas are colored equally
> > and not distinguishable.
> 
> Changed this to use the same colour as the QFB toolbar (darker than the
> rest). The unifinder-searchBox and the task-addition-box have now the same
> background colour to separate from the toolbar above.

The black background looks misplaced here. I think the grey parts from the light theme should just be a little darker then the parts that are white in the light theme. 

> > 2. The unifinder filter drowdown control in the calendar view has the
> > expected grey color when being collapsed but a white when being expanded.
> 
> This is expected as the menulist is active and open. Add the "Folder
> Location" menulist to the main toolbar and you see the same.

Firefox has also a dark background when opening the main menu aka hamburger button. Having a white area in a dark theme looks like an incomplete conversion to me.

> 
> > 4. In calendar view, the selectors for the view mode are already adopted to
> > the dark theme, but the selected view tab is still highlighted with a white
> > background.
> 
> I made this to better fit with the white content below. In main window the
> tabs do also fit with the toolbar colour. But I can change when you still
> want this.

I see, but I still would prefer a dark tab (since also the grid itself should be darkened at some time).

> > ::: calendar/base/themes/common/today-pane.css
> > @@ +244,2 @@
> > >    background-color: #DFEAF4;
> > > +  color: #000000;
> > 
> > Shouldn't be used theme color variables here instead of hard coding? That
> > would apply also to definitions above.
> 
> This items are already hard coded before. I had to add the text colour to
> not get the colour we set with the WE-theme.
> 
> If we want to change this to use systemColor too, we can do this in a new
> bug.

Yes, that would be nice.
Attached patch 1504190-themeCalendar.patch v7 (obsolete) — Splinter Review
(In reply to [:MakeMyDay] from comment #16)
> (In reply to Richard Marti (:Paenglab) from comment #8)
> > > 1. The parts of the todaypane with the default theme has areas with grey and
> > > white background color. With the dark theme, these areas are colored equally
> > > and not distinguishable.
> > 
> > Changed this to use the same colour as the QFB toolbar (darker than the
> > rest). The unifinder-searchBox and the task-addition-box have now the same
> > background colour to separate from the toolbar above.
> 
> The black background looks misplaced here. I think the grey parts from the
> light theme should just be a little darker then the parts that are white in
> the light theme. 

The light theme doesn't style this parts because it uses the default tree colours.

I changed it noe to use the same colours as the main toolbar and separated it with a border.

> > > 2. The unifinder filter drowdown control in the calendar view has the
> > > expected grey color when being collapsed but a white when being expanded.
> > 
> > This is expected as the menulist is active and open. Add the "Folder
> > Location" menulist to the main toolbar and you see the same.
> 
> Firefox has also a dark background when opening the main menu aka hamburger
> button. Having a white area in a dark theme looks like an incomplete
> conversion to me.

It's no system widget menulist popups are system widgets. Also when you right click in Firefox, the light context menu appears.Maybe I can do thie menulist popups in a other bug.

> > > 4. In calendar view, the selectors for the view mode are already adopted to
> > > the dark theme, but the selected view tab is still highlighted with a white
> > > background.
> > 
> > I made this to better fit with the white content below. In main window the
> > tabs do also fit with the toolbar colour. But I can change when you still
> > want this.
> 
> I see, but I still would prefer a dark tab (since also the grid itself
> should be darkened at some time).

Made it dark like the main tabs.

> > > ::: calendar/base/themes/common/today-pane.css
> > > @@ +244,2 @@
> > > >    background-color: #DFEAF4;
> > > > +  color: #000000;
> > > 
> > > Shouldn't be used theme color variables here instead of hard coding? That
> > > would apply also to definitions above.
> > 
> > This items are already hard coded before. I had to add the text colour to
> > not get the colour we set with the WE-theme.
> > 
> > If we want to change this to use systemColor too, we can do this in a new
> > bug.
> 
> Yes, that would be nice.

I'll do this then.
Attachment #9028424 - Attachment is obsolete: true
Attachment #9028424 - Flags: review?(makemyday)
Attachment #9029466 - Flags: review?
Attached patch 1504190-themeCalendar.patch v8 (obsolete) — Splinter Review
The calendar-nav-control has --lwt-accent-color set as background colour. So we need --lwt-header-image instead of --toolbar-bgimage.

And forgot to change the task-addition-box too.
Attachment #9029466 - Attachment is obsolete: true
Attachment #9029466 - Flags: review?
Attachment #9029627 - Flags: review?(makemyday)
Duplicate of this bug: 1517139

MakeMyDay, gentle review ping.

Comment on attachment 9029627 [details] [diff] [review]
1504190-themeCalendar.patch v8

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

Sorry for the delay. In general, it looks quite good already, but there are still some hover effects missing/wrong in the today pane and status bar for the dark theme (tested in Win):

No hover effect:
- new event button
- the twisty if expanded
- all controls in the mini-day
- text box to add tasks

Wrong hover effect:
- Today pane switcher in the status bar (just a border is displayed without changing the background color like in the Default theme)

Warning icon: the exclamation mark in the error triangle has no dot at the bottom (also true for default theme).

r- to get that still fixed.
Attachment #9029627 - Flags: review?(makemyday) → review-

(In reply to [:MakeMyDay] from comment #21)

Comment on attachment 9029627 [details] [diff] [review]
1504190-themeCalendar.patch v8

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

Sorry for the delay. In general, it looks quite good already, but there are
still some hover effects missing/wrong in the today pane and status bar for
the dark theme (tested in Win):

No hover effect:

  • new event button

Fixed

  • the twisty if expanded

The twisty in default has also hover effect.

  • all controls in the mini-day

Fixed

  • text box to add tasks

No textbox has a hover effect when themed. See also Firefox.

Wrong hover effect:

  • Today pane switcher in the status bar (just a border is displayed without
    changing the background color like in the Default theme)

Fixed

Warning icon: the exclamation mark in the error triangle has no dot at the
bottom (also true for default theme).

Fixed

Attachment #9029627 - Attachment is obsolete: true
Attachment #9041061 - Flags: review?(makemyday)

(In reply to Richard Marti (:Paenglab) from comment #22)

  • the twisty if expanded

The twisty in default has also hover effect.

Currently I see no hover effect with the twisties, either expanded or not. However, on ESR it exists - so probably an uncaught regression?

  • text box to add tasks

No textbox has a hover effect when themed. See also Firefox.

It has in default theme - it's the border color to be precise.

(In reply to [:MakeMyDay] from comment #23)

(In reply to Richard Marti (:Paenglab) from comment #22)

  • the twisty if expanded

The twisty in default has also hover effect.

The no was missing in my sentence above.

Currently I see no hover effect with the twisties, either expanded or not. However, on ESR it exists - so probably an uncaught regression?

This was removed in the toolkit tree unification.

  • text box to add tasks

No textbox has a hover effect when themed. See also Firefox.

It has in default theme - it's the border color to be precise.

I can look in a new bug if I can add a hover effect. But this affects all themeable textboxes.

Comment on attachment 9041061 [details] [diff] [review]
1504190-themeCalendar.patch

Ok thanks. Can you please still file and reference the follow-up bugs mentioned in comment 8 and comment 24 when landing this?
Attachment #9041061 - Flags: review?(makemyday) → review+
Keywords: checkin-needed

(In reply to [:MakeMyDay] from comment #25)

Comment on attachment 9041061 [details] [diff] [review]
1504190-themeCalendar.patch

Ok thanks. Can you please still file and reference the follow-up bugs
mentioned in comment 8 and comment 24 when landing this?

Filed bug 1524921 and bug 1524922.

Pushed by richard.marti@gmail.com:
https://hg.mozilla.org/comm-central/rev/9e2f7aadb737
Make the Calendar themeable with WX-themes. r=MakeMyDay

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

This should be target 6.9.

Target Milestone: --- → 6.8
Attachment #9041061 - Flags: approval-calendar-beta?(philipp)
Target Milestone: 6.8 → 6.9
Attachment #9041061 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
You need to log in before you can comment on or make changes to this bug.