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



2 years ago
Last year


(Reporter: jorgk, Assigned: martinschroeder)


Lightning 5.8
Dependency tree / graph



(2 attachments, 3 obsolete attachments)



2 years ago
Posted 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.

Comment 2

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

Comment 3

2 years ago
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.

Comment 5

2 years ago
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

Comment 6

2 years ago
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


2 years ago
Duplicate of this bug: 1410173
Blocks: ltn62
Assignee: nobody → mschroeder
Posted 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+

Comment 10

Last year
Martin, do you still intend to fix this for 6.2?
Flags: needinfo?(mschroeder)
Posted patch Fixes (part 2) v2 (obsolete) — Splinter Review
Attachment #8944066 - Attachment is obsolete: true
Flags: needinfo?(mschroeder)
Attachment #8966060 - Flags: review?(makemyday)

Comment 12

Last year
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)

Comment 14

Last year
Pushed by
Fix various JS errors in Calendar [ReferenceError: document is not defined]. r=MakeMyDay
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #8969998 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+

Comment 15

Last year
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.