DATE values must not specify a TZID

VERIFIED FIXED in 0.8

Status

Calendar
Internal Components
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

unspecified
Bug Flags:
blocking-calendar0.8 +

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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.
Flags: blocking-calendar0.7?
(Assignee)

Updated

11 years ago
Depends on: 398724
(Assignee)

Updated

11 years ago
Flags: blocking-calendar0.7? → wanted-calendar0.8+
Version: Sunbird 0.5 → unspecified
(Assignee)

Comment 1

11 years ago
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.
Assignee: nobody → michael.buettner

Updated

11 years ago
Flags: wanted-calendar0.8+ → blocking-calendar0.8+

Comment 2

11 years ago
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. 
(Assignee)

Comment 3

11 years ago
(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

11 years ago
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?
(Assignee)

Comment 5

11 years ago
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).
(Assignee)

Updated

11 years ago
Assignee: michael.buettner → daniel.boelzle

Comment 6

11 years ago
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)
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 299189 [details] [diff] [review]
fix
Attachment #299189 - Flags: review?(michael.buettner)
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.
Attachment #299189 - Flags: review?(michael.buettner) → review+
(Assignee)

Comment 10

11 years ago
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*.
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.
(Assignee)

Comment 12

11 years ago
Created attachment 299205 [details] [diff] [review]
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.
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... :-)
(Assignee)

Comment 14

11 years ago
(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()
(Assignee)

Comment 15

11 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8

Comment 16

11 years ago
Checked in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.