1399842 - Fix various JS errors in Calendar [ReferenceError: document is not defined]

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: more-js-errors.patch - WIP

+++ This bug was initially created as a clone of Bug #1399483 +++

See bug 1399483 comment #4:
ReferenceError: Services is not defined  datetimepickers.xml:1786:15
ReferenceError: Services is not defined  datetimepickers.xml:1878:15
ReferenceError: document is not defined calUtils.js:1152:21
ReferenceError: document is not defined calUtils.js:304:9
TypeError: this.schangeMenuByPropertyName is not a function  calendar-menus.xml:23:38

The attached patch fixed the Services and the schangeMenuByPropertyName, the document error is not solved. Stefan said:

The changes in calutils.js have no impact, I now get ReferenceError: window is not defined instead. The error is reported when opening context menu for task entry in today pane and opening e.g. Progress sub-menu.

Comment 1

[Stefan Sitter [:ssitter]]

The 'Services is not defined' errors are reported when opening the Add Custom Reminder dialog from the New Event dialog. Working with this dialog produced another error:

TypeError: this.parseTimeRegExp is undefined
	parseTime chrome://calendar/content/datetimepickers/datetimepickers.xml:1624:15
	parseTextBoxTime chrome://calendar/content/datetimepickers/datetimepickers.xml:610:22

The 'document is not defined' calUtils.js:304 error is reported when opening the Print dialog and choosing something in the date picker.

The 'document is not defined' calUtils.js:1152 error is reported when opening the context menu for task entry in today pane.

Comment 2

[Jorg K (CEST = GMT+2)]

Can you do a patch? They predicted that I'd become a Calendar contributor, but I'm resisting ;-)

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1399483-more-js-errors.patch (v1)

OK, this fixes Services, parseTimeRegExp and schangeMenuByPropertyName.

I have no idea about the document error. Looking at the code, getElementById() is always called on the document, and I doubt that that worked before.

So should we land this as a partial fix and ask the Calendar gurus (who are not available easily) about the document?

Comment 4

[Martin Schröder [:mschroeder]]

The problem is probably the inclusion of calUtils.js in calUtils.jsm which triggers the errors because of 'strict' now being used for modules. We should be able to inline calUtils.js' calRadioGroupSelectItem() function at its only usage. applyAttributeToMenuChildren() has a total of three usages in two files, and maybe it is okay to duplicate the code.

Comment 5

[Jorg K (CEST = GMT+2)]

Could you submit a patch please. I'm going to land the first patch now. I'm not so familiar with Calendar code, I'm just trying to help out and fix the obvious. BTW, next week is branch day, so it would be good to settle this soon. Nice to have a reviewer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cbe0e3a04543
Backed out changeset 68db819d0e77 for landing with the wrong bug number: Bug 1399842 and not bug 1399483. a=backout
https://hg.mozilla.org/comm-central/rev/95ed66b5bdec
fix some calendar JS errors (typo, missing import). r=ssitter DONTBUILD

Comment 8

[Martin Schröder [:mschroeder]]

Attachment: Fixes (part 2)

Untested changes for removing the remaining errors.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8944066 [details] [diff] [review]
Fixes (part 2)

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

Looks right to me. When you have some time can you give it a test? Do you want to land this before or after bug 1433229

::: calendar/base/content/dialogs/calendar-print-dialog.js
@@ +312,5 @@
> +            index = i;
> +            break;
> +        }
> +    }
> +    ASSERT(index && index != 0, "Can't find radioGroup item to select.", true);

cal.ASSERT

Comment 10

[[:MakeMyDay]]

Martin, do you still intend to fix this for 6.2?

Comment 11

[Martin Schröder [:mschroeder]]

Attachment: Fixes (part 2) v2

Comment 12

[[:MakeMyDay]]

Comment on attachment 8966060 [details] [diff] [review]
Fixes (part 2) v2

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

Thanks, looks good. r+ with the comments considered.

::: calendar/base/content/dialogs/calendar-print-dialog.js
@@ +312,5 @@
> +            index = i;
> +            break;
> +        }
> +    }
> +    cal.ASSERT(index && index != 0, "Can't find radioGroup item to select.", true);

Do we still need an assertion here? Just putting an if caluse around the line below should suffice.

But even if so, the message could now provide more context, since its a local function now.

::: calendar/base/modules/calUtilsCompat.jsm
@@ -85,5 @@
>      },
>      view: {
>          isMouseOverBox: "isMouseOverBox",
> -        calRadioGroupSelectItem: "radioGroupSelectItem",
> -        applyAttributeToMenuChildren: "applyAttributeToMenuChildren",

When removing this here, please add a comment likewise for the removed cal.unifinder.* functions:

https://dxr.mozilla.org/comm-central/source/calendar/base/modules/calUtilsCompat.jsm#113

Comment 13

[Martin Schröder [:mschroeder]]

Attachment: Fixes (part 2) v3

Carrying over r+.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3eb0a15e763e
Fix various JS errors in Calendar [ReferenceError: document is not defined]. r=MakeMyDay

Comment 15

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 5, Calendar 6.2):
https://hg.mozilla.org/releases/comm-beta/rev/5461aa9fc88ca52b75e830cca8ca6ca2a9e05ff8