794585 - Monthly layout: unnecessary additional empty week row printed if last day of month is last day of print range

Status: RESOLVED FIXED

(Calendar :: Printing, defect)

Description

[Stefan Sitter [:ssitter]]

An unnecessary additional empty week is printed in Monthly layout if last day of month is last day of print range. See attached screenshot. The monthly calendar view on the left is correct, the monthly print output on the right contains the unnecessary additional empty week row.

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: screenshot

Comment 2

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

Attachment: Fix - v1

This should do it, I'd appreciate if you could give it a spin.

Comment 3

[Stefan Sitter [:ssitter]]

Comment on attachment 666909 [details] [diff] [review]
Fix - v1

Patch is empty.

Comment 4

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

Attachment: Fix - v1

Sorry about that

Comment 5

[Stefan Sitter [:ssitter]]

Comment on attachment 666924 [details] [diff] [review]
Fix - v1

From a first test at least two problems:

1) Weekly print layout seems completely broken with the patch, shows only blank page and the following error:

> Error: 'TypeError: parentNode is undefined' when calling method:[calIPrintFormatter::formatToHtml] = NS_ERROR_XPC_JS_THREW_JS_OBJECT
> Source file: chrome://calendar/content/calendar-print-dialog.js Line: 208

2) Monthly print layout testcase fails:

First day of week is Sunday (default). Calendar view is set to month September and shows 6 weeks (26.08.2012 - 06.10.2012).

Without patch: Print output in monthly format shows the very same -> OK.

With patch: Print output doesn't contain the last day of September anymore, shows only 5 weeks (26.08.2012 - 29.09.2012). In addition a second calendar is printed for October that contains 5 more weeks (30.09.2012 - 03.11.2012).

Comment 6

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

I don't understand :-( parentNode is the weekContainer, which is a html element on the template. Why would changing the weekEnd (which is evaluated after getting the element) remove the weekContainer!?

I'm sorry, but I'll have to take care of this after the release...

Comment 7

[Stefan Sitter [:ssitter]]

Out of curiosity: Why was userWeekStart() and userWeekEnd() introduced for the printing code? Can't we just reuse getStartOfWeek() and getEndOfWeek() from calWeekInfoService?

Comment 8

[Stefan Sitter [:ssitter]]

I'm working on it.

Comment 9

[Stefan Sitter [:ssitter]]

Attachment: fix for monthly and weekly print

Patch uses the existing functions to get first and last day of the week and the removes the duplicates that were added with Bug 757902.

Patch was tested mostly for Sunday and Monday as first day of week, so far it looks good. 

The TypeError in Comment 5 happened only if Monday was selected as first day of week. I think it was caused by the usage of calIDateTime::startOfWeek that always returns the Sunday of the week. I assume this somehow messed up the created document.

It seems that fixing this error fixed the behavior described in Bug 792061 too.

Comment 10

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

Comment on attachment 668155 [details] [diff] [review]
fix for monthly and weekly print

Looks good, I've also done some simple tests and it seems to work. Approval for aurora, if you push this after the merge (which is today, or was today, not sure) then also approval for beta.

Comment 11

[Stefan Sitter [:ssitter]]

Pushed to https://hg.mozilla.org/comm-central/rev/c311d81476a4

Comment 12

[Stefan Sitter [:ssitter]]

Pushed to https://hg.mozilla.org/releases/comm-aurora/rev/1f1c1c48c3b3