1558384 - All-day events in Today Pane are missing their background colours

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: example

I think this might be something to do with bug 1504416, as it's the calendar list that sets the style rules for calendar colours. The colours appear after switching to the calendar tab for the first time.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1558384-css-colour-vars-1.diff

This is a completely new approach to item colouring that I've wanted to implement for some time. It uses CSS variables rather than dynamically-added CSS rules to track colours. I've made a singleton object to keep track of all calendars and all windows that have the rules. All that's required to use it in a window is to register the window.

This doesn't currently deal with the calendar list tree, as that's a tree and they have weird CSS ways. I plan to stop using tree for that in the near future and hook it up to this mechanism. It also doesn't yet cover the printing use of CSS rules, although that should be easy.

I think it would also be very simple to do the item category colours this way.

For now, I've done enough to fix the bug.

Comment 2

[Stefan Sitter [:ssitter]]

Seems to work for me in recent 68 beta build. Maybe you were affected by Bug 1558084? It caused e.g. calendar color changes to not propagate to the trees.

Comment 3

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

Comment on attachment 9071559 [details] [diff] [review]
1558384-css-colour-vars-1.diff

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

Does this regress the colors on any of the other items you said weren't hooked up yet? If you aren't doing the remaining items in this bug, then you should file a followup and take care of it soon, otherwise it will stay so until the next thing breaks.

::: calendar/base/content/agenda-listbox.js
@@ +315,2 @@
>              let imagebox = this.querySelector(".agenda-calendar-image");
> +            imagebox.style.backgroundColor = `var(--calendar-${this.mOccurrence.calendar.id}-backcolor)`;

While we set it to something generic, the calendar id is in theory free form. Can you apply som escaping so we don't break any css rules or colors? I think there may be a `formatStringForCSSRule` function that you could use or adapt.

::: calendar/base/modules/utils/calViewUtils.jsm
@@ +298,5 @@
> +
> +    // The only public method. Deregistration is not required.
> +    registerWindow(aWindow) {
> +        if (this.calendars === null) {
> +            // QI added lazily so that we don't cause an import loop.

Can you elaborate why the import loop?

::: calendar/lightning/content/messenger-overlay-sidebar.js
@@ +375,5 @@
>      tabmail.registerTabType(calendarTabType);
>      tabmail.registerTabType(calendarItemTabType);
>      tabmail.registerTabMonitor(calendarTabMonitor);
>  
> +    cal.view.colorTracker.registerWindow(window);

This might be a good fit for https://searchfox.org/comm-central/source/calendar/base/content/calendar-chrome-startup.js

Comment 4

[Jorg K (CEST = GMT+2)]

Geoff, can you get this ready for check-in. I'd like to see whether it fixes the issue shown in attachment 9072434 [details].

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1558384-css-colour-vars-2.diff

Comment 6

[Geoff Lankow (:darktrojan)]

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

Does this regress the colors on any of the other items you said weren't
hooked up yet? If you aren't doing the remaining items in this bug, then you
should file a followup and take care of it soon, otherwise it will stay so
until the next thing breaks.

No regression that I know of, and I have 2 out of 3 remaining parts ready now.

While we set it to something generic, the calendar id is in theory free
form. Can you apply som escaping so we don't break any css rules or colors?
I think there may be a formatStringForCSSRule function that you could use
or adapt.

Done.

Can you elaborate why the import loop?

In fact there isn't a loop, that was an assumption I clearly didn't check out properly.

This might be a good fit for
https://searchfox.org/comm-central/source/calendar/base/content/calendar-
chrome-startup.js

Moved.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b58f19c12f22
Use CSS variables for calendar item colours rather than dynamically adding CSS rules. r=philipp DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

TB 68 beta 3 / Cal 7.0:
https://hg.mozilla.org/releases/comm-beta/rev/63eb38dd6c031aab65eea471e88cca1d84dc7287

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #4)

... I'd like to see whether it fixes the issue shown in attachment 9072434 [details].

I haven't seen this issue any more in TB 68 beta 3.