388656 - DATE values must not specify a TZID

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Daniel Boelzle [:dbo]]

Section 3.8.3.1 (Time Zone Identifier) of <http://www.ietf.org/internet-drafts/draft-ietf-calsify-rfc2445bis-07.txt> specifies that TZID must only be specified on properties of value type DATE-TIME or TIME, but not DATE. Our products currently write a TZID for DATE values.

Comment 1

[Daniel Boelzle [:dbo]]

I am unsure whether we should remove a TZID param from existing DATE properties: strictly spoken this is dataloss. But we should omit writing a TZID when the user changes an event/task to be all-day in the event/task dialog.

Comment 2

[Bas van den Bosch]

No problem with deleting the TZID from the DATE when changing events as we adhere to the spec and the behaviour stays the same. However, if you don't have a tzid on the all-day event (DATE) and the event gets changed to a non-allday-event I think the event might go floating while it should jump to the TZ from the preferences (or even th epreviously assigned TZID like it does now). Didn't try this yet though, this might change since with the foreign timezone fix. 

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> non-allday-event I think the event might go floating while it should jump to
> the TZ from the preferences (or even th epreviously assigned TZID like it does
> now). Didn't try this yet though, this might change since with the foreign
Yes, makes sense. Non-Allday events should get a sensible timezone by default.

Comment 4

[cmtalbert]

What's the prospect for a patch on this little monster? Are we going tohave to change libical or are we going to enforce this at the CalDateTime layer?

Comment 5

[Daniel Boelzle [:dbo]]

I propose that we leave data "as is" unless the user changes an item. That said I think it's event/task dialog work (only).

Comment 6

[Bas van den Bosch]

The "TZID" property parameter MUST NOT be applied to DATE properties, and DATE-TIME or TIME properties whose time values are specified in UTC.

is specifically stated in rfc2445-bis07 so I suggest dropping the TZID and adding the default whenever an event has a DATE-TIME or TIME without a corresponding timezone (we want to avoid floating events I guess)

Comment 7

[Daniel Boelzle [:dbo]]

Bas, you're right, but please think about calendar interop with applications that may still interpret the TZID in some way. Being defensive, I'd just strip the TZID from DATEs when actually edited.

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: fix

Comment 9

[Michael Büttner (no reviews TFN)]

Comment on attachment 299189 [details] [diff] [review]
fix

>+function onUpdateAllDay() {
updateAllDay() is the dedicated handler for the allday-checkbox, there's no need to introduce another function that calls updateAllDay(). I suggest to merge the body of this new function into updateAllDay().

>+    if (!isEvent(window.calendarItem)) {
>+        return;
>+    }
This check will be performed in updateAllDay(), no need to do that twice.

>+    var allDay = getElementValue("event-all-day", "checked");
Same here, already contained in updateAllDay().

>+    gStartTimezone = (allDay ? floating(): calendarDefaultTimezone());
>+    gEndTimezone = gStartTimezone;
>+    gStartTime.timezone = gStartTimezone;
>+    gEndTime.timezone = gEndTimezone;
These four lines should just be moved into updateAllDay(), that's it.

Apart from the above mentioned minor restructuring the patch looks just fine. r=mickey.

Comment 10

[Daniel Boelzle [:dbo]]

Mickey, please rethink your review.
Initially I've added the code to updateAllday(), but it regressed other stuff. updateAllday() is called from lots of other update functions throughout the file, even recursively (controlled via that weird global gIgnoreUpdate variable, control flow is quite hard to predict IMHO).
So I moved it into that new function which is called on actual user action *only*.

Comment 11

[Michael Büttner (no reviews TFN)]

Just because the call flow is probably not immediately obvious nor trivial this isn't a good reason for taking the easy road. Feel free to ignore my review, but I carefully thought about what I wrote in my previous comment.

Comment 12

[Daniel Boelzle [:dbo]]

Attachment: suggestion

Is this what you've proposed? I respect your input, but it doesn't WFM since a all events' timezones will be forced to calendarDefaultTimezone() in the event dialog.

Comment 13

[Michael Büttner (no reviews TFN)]

I don't exactly understand your previous comment. Did you mean that with the proposed patch the timezone will always be set to calendarDefaultTimezone()? This shouldn't be the case and I can't find a location that unconditionally revert to the default timezone. Dumb question, but are we talking about the timezone option being enabled or disabled? If it's enabled all timezones should be taken from gStart/EndTimezone, otherwise we always default to the default timezone. At least the proposed patch is what I had in mind previously... :-)

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #13)
> I don't exactly understand your previous comment. Did you mean that with the
> proposed patch the timezone will always be set to calendarDefaultTimezone()?
Yes. It's displayed that way even if the event's timezone is different.

> This shouldn't be the case and I can't find a location that unconditionally
> revert to the default timezone. Dumb question, but are we talking about the
> timezone option being enabled or disabled? If it's enabled all timezones should
> be taken from gStart/EndTimezone, otherwise we always default to the default
> timezone. At least the proposed patch is what I had in mind previously... :-)
mabye it's triggered via updateCalendar()

Comment 15

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.

Comment 16

[Andreas Treumann]

Checked in latest nightly build -> task is fixed and verified.