Closed Bug 1399842 Opened 4 years ago Closed 3 years ago

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


(Calendar :: General, defect)

Lightning 5.8
Not set


(Not tracked)



(Reporter: jorgk-bmo, Assigned: mschroeder)




(2 files, 3 obsolete files)

Attached patch more-js-errors.patch - WIP (obsolete) — Splinter Review
+++ 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.
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.
Can you do a patch? They predicted that I'd become a Calendar contributor, but I'm resisting ;-)
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?
Attachment #8908093 - Attachment is obsolete: true
Attachment #8908169 - Flags: review?(ssitter)
Attachment #8908169 - Flags: review?(ssitter) → review+
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.
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.
Keywords: leave-open
Pushed by
Backed out changeset 68db819d0e77 for landing with the wrong bug number: Bug 1399842 and not bug 1399483. a=backout
fix some calendar JS errors (typo, missing import). r=ssitter DONTBUILD
Target Milestone: --- → 5.9
Summary: Fix various JS errors in Calendar → Fix various JS errors in Calendar [ReferenceError: document is not defined]
Version: Trunk → Lightning 5.8
Duplicate of this bug: 1410173
Blocks: ltn62
Assignee: nobody → mschroeder
Attached patch Fixes (part 2) (obsolete) — Splinter Review
Untested changes for removing the remaining errors.
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);

Attachment #8944066 - Flags: feedback+
Martin, do you still intend to fix this for 6.2?
Flags: needinfo?(mschroeder)
Attached patch Fixes (part 2) v2 (obsolete) — Splinter Review
Attachment #8944066 - Attachment is obsolete: true
Flags: needinfo?(mschroeder)
Attachment #8966060 - Flags: review?(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:
Attachment #8966060 - Flags: review?(makemyday) → review+
Carrying over r+.
Attachment #8966060 - Attachment is obsolete: true
Attachment #8969998 - Flags: review+
Target Milestone: 5.9 → 6.3
Attachment #8969998 - Flags: approval-calendar-beta?(philipp)
Pushed by
Fix various JS errors in Calendar [ReferenceError: document is not defined]. r=MakeMyDay
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #8969998 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Beta (TB 60 beta 5, Calendar 6.2):
Target Milestone: 6.3 → 6.2
You need to log in before you can comment on or make changes to this bug.