1717637 - Remove unneeded global.css imports in calendar files and make remaining dialogs themeable

Status: RESOLVED FIXED

(Calendar :: Dialogs, task)

Description

[Richard Marti (:Paenglab)]

I saw that some dialogs imported global.css rules twice. This came from importing global.css and messenger.css which itself also imports global.css.

I removed global.css and added messenger.css in conjunction with adding themeable support for the dialogs that haven't yet.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1717637-calendar-remove-global.patch

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9228351 [details] [diff] [review]
1717637-calendar-remove-global.patch

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

This is a great improvement, thanks.
There are some minor issues that I'm not sure are related to these changes, but I think we should tackle here.

Uploading screenshots and description in a minute.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-06-22 10-49-57.png

Alert dialogs on Linux with TB dark theme don't have readable buttons.
This happens also for the "Attach link" dialog.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-06-22 10-46-38.png

The lock button between start/end date doesn't have a good hover color when dark theme is used.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1717637-calendar-remove-global.patch

The save dialog is M-C issue.
Fixed the keepduration-button.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9228426 [details] [diff] [review]
1717637-calendar-remove-global.patch

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

Visually looks good.
Some things to fix.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.xhtml
@@ +36,5 @@
> +    <script src="chrome://calendar/content/import-export.js"/>
> +    <script src="chrome://calendar/content/widgets/calendar-item-summary.js"/>
> +    <script src="chrome://calendar/content/calendar-dialog-utils.js"/>
> +    <script src="chrome://calendar/content/calendar-ui-utils.js"/>
> +    <script src="chrome://messenger/content/dialogShadowDom.js"/>

Magnus usually points out that the script tag shouldn't be self closing.
Let's ping him to confirm before landing this.

::: calendar/base/content/dialogs/publishDialog.xhtml
@@ +13,5 @@
>    <!ENTITY % dtd1 SYSTEM "chrome://calendar/locale/global.dtd" > %dtd1;
>    <!ENTITY % dtd2 SYSTEM "chrome://calendar/locale/calendar.dtd" > %dtd2;
>  ]>
>  
> +<window id="calendar-publishwindow"

You can't move the ID from the dialog to the Windows as it breaks all those methods in the publishDialog.js file that use the ID to find the buttons.

::: calendar/base/themes/common/calendar-alarms.css
@@ +2,5 @@
>   * 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/. */
>  
> +.reminder-details > .alarm-icons-box {
> +  width: 15px;

This should have 4 spaces indentation if we want to respect this file current indentation.
Or maybe we should use the occasion to move everything to 2 spaces.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: 1717637-calendar-remove-global.patch

(In reply to Alessandro Castellani [:aleca] from comment #6)

Comment on attachment 9228426 [details] [diff] [review]
1717637-calendar-remove-global.patch

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

Visually looks good.
Some things to fix.

::: calendar/base/content/dialogs/calendar-ics-file-dialog.xhtml
@@ +36,5 @@

• <script src="chrome://calendar/content/import-export.js"/>
• <script src="chrome://calendar/content/widgets/calendar-item-summary.js"/>
• <script src="chrome://calendar/content/calendar-dialog-utils.js"/>
• <script src="chrome://calendar/content/calendar-ui-utils.js"/>
• <script src="chrome://messenger/content/dialogShadowDom.js"/>

Magnus usually points out that the script tag shouldn't be self closing.
Let's ping him to confirm before landing this.

The other dialogs hadn't the closing </script> and wanted them consistent. Self closing is only an issue when it is a HTML. Reverted my changes.

::: calendar/base/content/dialogs/publishDialog.xhtml
@@ +13,5 @@

<!ENTITY % dtd1 SYSTEM "chrome://calendar/locale/global.dtd" > %dtd1;
<!ENTITY % dtd2 SYSTEM "chrome://calendar/locale/calendar.dtd" > %dtd2;
]>

+<window id="calendar-publishwindow"

You can't move the ID from the dialog to the Windows as it breaks all those
methods in the publishDialog.js file that use the ID to find the buttons.

This dialog wasn't changed in bug 1698143 and I wanted to make persist again. Are my changes in publishDialog.js okay to work again?

::: calendar/base/themes/common/calendar-alarms.css
@@ +2,5 @@

• 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/. */

+.reminder-details > .alarm-icons-box {

• width: 15px;

This should have 4 spaces indentation if we want to respect this file
current indentation.
Or maybe we should use the occasion to move everything to 2 spaces.

Changed to 4 spaces indentation. We should the whole conversion do in a separate bug. This one is already big enough.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9228491 [details] [diff] [review]
1717637-calendar-remove-global.patch

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

Seems to be good, thanks.
Maybe a try-run to be sure we're not introducing any regression? Pretty unusual but not impossible :D

Comment 9

[Richard Marti (:Paenglab)]

Try: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=59a06e07fb44fe1089ed800a00592acfd251d67b

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0ae48a8d8582
Remove unneeded global.css imports in calendar files and make remaining dialogs themeable. r=aleca