Closed Bug 351380 Opened 13 years ago Closed 13 years ago

wrong day selected after inserting a day name in the datetextpicker

Categories

(Calendar :: Lightning Only, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: mattwillis)

Details

Attachments

(2 files, 2 obsolete files)

- go to the currend week 36 and select Thusday 09/05/2006 in the calendar ui
- insert 'friday' in the datetextpicker beneath the mini month
- press the 'Go' button
- the text in the text field changed to 09/09/2006 and Saturday is selected in the calendar ui and not Friday, 09/08/2006.

Everytime you insert a day name in the datetextpicker and push the 'Go' button the next day gets selected.
Maybe a blocking bug for 0.3, because the datetextpicker is a new feature. 
Flags: blocking0.3?
Probably related to timezones.  What timezone are you in and what time was it when you found this bug?
My timezone is Europe/Berlin, and I also selected this timezone in Thunderbird at 'Tools/Options/Timezone.
I found this bug yesterday at 4 pm. It was possible form me to repruduce this issue today at 9 am.
Flags: blocking0.3? → blocking0.3+
I think the problem is caused by being to cautious:

In /calendar/resources/content/datetimepickers/datetimepickers.xml in constructor the day names are stored in an internal array. That means
  this.mDayNames[0] = day.1.name = Sunday
  this.mDayNames[1] = day.2.name = Monday
  ...

In parseLanguageDate() it is assumed that i-weekday has offset (off by 1) and +1 is added to every day. But that is not true because i is zero-based:

  Tuesday == this.mDayNames[2] --> i = 2 
  Today is Tuesday --> calDate.weekday = 2
  offset = (2 - 2 + 7) % 7 + 1 = 1
  Today/Tuesday + 1 = Wednesday --> all days are off by one
  
Removing the +1 to offset should fix the problem.
(In reply to comment #5)
> Removing the +1 to offset should fix the problem.
> 
Not quite, I don't think.  The +1 was necessary for the days to be correct for me.  I think the problem is actually jsDate lossage in http://lxr.mozilla.org/mozilla/source/calendar/resources/content/datetimepickers/datetimepickers.xml#176 when we try to get the current weekday.  The +1 probably needs to go, once we fix that, though.
Whiteboard: [no l10n impact]
Whiteboard: [no l10n impact] → [no l10n impact][needs patch]
(In reply to comment #6)
I retested with both OS and Lightning timezone set to Los Angeles and I'm still getting the same trace results as in Comment #5.
Ok after more testing I think jminta is right. Based on the timezones UTC offset the calculated offset is off by +1 or -1 if your local time is between 00:00 and 00:00+UTC offset. Some example as the text picker is working now:

Timezone is to Europe/Berlin (UTC + 2):
 local time is 00:05 -> enter monday -> wednesday is selected (+2)
 local time is 01:55 -> enter monday -> wednesday is selected (+2)
 local time is 02:05 -> enter monday -> tuesday is selected (+1)
 local time is 20:05 -> enter monday -> tuesday is selected (+1)
 local time is 23:55 -> enter monday -> tuesday is selected (+1)

Timezone is to Los Angeles (UTC - 7):
 local time is 00:05 -> enter monday -> tuesday is selected (+1)
 local time is 08:05 -> enter monday -> tuesday is selected (+1)
 local time is 16:55 -> enter monday -> tuesday is selected (+1)
 local time is 17:05 -> enter monday -> monday
 local time is 18:05 -> enter monday -> monday
 local time is 23:05 -> enter monday -> monday

So the +1 that is added is a workaround for this problem when local timezone is e.g. UTC-7 and if tested between 17:00 and 00:00. During rest of the day the calulation is wrong too. In local timezone with UTC+xx this results in wrong calculated offset that are off by +1 or +2.
This should not block 0.3. We might just back out the natural language parsing. It was never meant to be in 0.3. (I only just found out about it, in an unrelated bug)
(besides: it doesn't work at all for me. No matter what i type, it always ends up with today)
It'd be nice to have, but we could live without it.
Flags: blocking0.3+
Joey talked me into making this block again since it's very visible.  However, I believe mvl is on the right track here: we should choose the most expedient solution from these two options:

1) hide the UI
2) fix the bug
Flags: blocking0.3+
I'd vote for hiding this feature. I didn't even know that it's possible to enter names of weekdays in order to find the corrosponding date. furthermore, the behavior is pretty confusing, even if it would work as expected.

it could well be possible that i didn't realize the bug where this feature has been introduced, but as far as i remember we all agreed that new (ui) features are to be discussed at the newsgroups / proposed on the wiki. probably i had a total blackout and missed the discussion, but i can't remember to read anything about this feature. so i'm really wondering how this could have been introduced in the first place.
Dan: this is not all that visible, given that it is not documented anywhere that you can type in day names.

Michael: This new feature was never discussed. I didn't even knew it existed. It was added as part of a different feature, a go-to-date for lightning. Adding the normal datapicker was ui-reviewed, adding features to the datepicker wasn't. I don't know why not.
Given that this code uses timezones and still tries to use jsdate, fixing it will cause a lot of headaches. Let's just hide the feature.
Patch disables the (never properly reviewed) attempt to do language parsing.
Attachment #239017 - Flags: second-review?(dmose)
Attachment #239017 - Flags: first-review?(mattwillis)
Comment on attachment 239017 [details] [diff] [review]
backout language parsing (checked in)

r1=lilmatt

Let's not waste anymore time on this before 0.3
Attachment #239017 - Flags: first-review?(mattwillis) → first-review+
Attached patch remove entire UI (obsolete) — Splinter Review
Alternative patch: remove the entire UI. Looks somewhat nicer.
A couple of points here:
-The first patch would create a drop-down picker (which will be broken on mac), which is redundant with the minimonth that is right above it.  That seems like questionable UI to me, and this patch should probably go through the UI-review process.
-The original patch was properly reviewed.  It had 2 reviews and a ui-review+ from dmose.
-The second patch regresses the original bug, which was to provide a jump-to-date functionality (especially go-to-today) for Lightning.  That (blocking) bug would need to be reopened if the second patch is checked in.
The addition of a datapicker was indeed reviewed. But that was before a language parser was added. I would not have let that go so easy, because parsing languages is hard. It needs more discussion and input from localizers.
There also needs to be dicussion about the behaviour of the text parser, for example to answer question like 'what does it mean if i type in yesterday? is that yesterdat based on the current date, or based on the currently visible day?'

Given all this, a third alternative is possible: leave the UI, but remove the attempt to parse text, only leaving the ability to type in a date using numbers.
Sorry for not jumping in here sooner.  I gave this ui-review as Lightning UI owner because to me it seemed like something that most folks would be on-board with.  I agree with mvl's statement that "language parsing is hard", but only in the general case.  It's my belief that it's very tractable in a case such as this where the domain is tightly-constrained.  That said, it sounds like my estimation of general on-boardness was wrong, so I think what mvl proposes in comment 19 is the right thing to do for 0.3, and we can have a more public discussion about the language stuff after that.
Whiteboard: [no l10n impact][needs patch] → [no l10n impact][needs review dmose]
Comment on attachment 239017 [details] [diff] [review]
backout language parsing (checked in)

r2=dmose
Attachment #239017 - Flags: second-review?(dmose) → second-review+
Whiteboard: [no l10n impact][needs review dmose] → [no l10n impact][needs checkin mvl]
lilmatt pointed out that there is also complication here due to the fact that month/day ordering varies, and even if we're internally consistent, pasting from an email or web page might well be using the other ordering.  I think we will do well to have more discussion and design stuff before turning this on.
Whiteboard: [no l10n impact][needs checkin mvl] → [no l10n impact][needs checkin]
Comment on attachment 239030 [details] [diff] [review]
remove entire UI

Not going to use this approach
Attachment #239030 - Attachment is obsolete: true
Comment on attachment 239017 [details] [diff] [review]
backout language parsing (checked in)

Patch (with spacer fix) checked in on MOZILLA_1_8_BRANCH and trunk.
Attachment #239017 - Attachment description: backout language parsing → backout language parsing (checked in)
Checking attachment 239017 [details] [diff] [review] in has removed the feature from Lightning for 0.3.
Removing blocking0.3 and leaving open to fix the datetextpicker for real.
Flags: blocking0.3+
Hardware: PC → All
Whiteboard: [no l10n impact][needs checkin]
Joey Mintas comment 18 is correct: the dropdown datepicker is redundant and entering a date by hand + pressing enter does not work. Either remove this box entirely or add ability to enter dates and press enter.
Based on the fact that the datetimepicker is A) broken in language parsing B) not switching selected day in view when being clicked C) not reacting on keyboard input and D) redundant I think we need a solution for the next release -> requesting blocking‑calendar0.5 flag.
Flags: blocking-calendar0.5?
We should pull this for 0.5.
Flags: blocking-calendar0.5? → blocking-calendar0.5+
I'd vote for removing the datetimepicker completely. Any objections?
We'd like some "go to date" functionality in Ln, but clearly the datetextpicker as it stands doesn't fulfill that.  I'm going to turn it off on branch (again) for 0.5 and we can revisit the language parsing stuff later.
I would like to poke Christian for some options on how we could integrate the "go to date" functionality. I can't imagine that a natural language parsing strategy is really what's needed here. For sure, there are easier to implement but much more usable options available.
Attached patch Remove busted datepicker (obsolete) — Splinter Review
Attachment #258662 - Flags: first-review?
Attachment #258662 - Flags: first-review? → first-review?(michael.buettner)
Comment on attachment 258662 [details] [diff] [review]
Remove busted datepicker

you probably want to address this here, too... ;-)
http://lxr.mozilla.org/seamonkey/source/calendar/lightning/content/messenger-overlay-sidebar.js#225
Attachment #258662 - Flags: first-review?(michael.buettner) → first-review-
This fixes the actual bug with the datetextpicker.  I'm spinning off the "do we turn it on in Lightning" issue and the "the current datepicker doesn't work" issues to other bugs.
Assignee: nobody → lilmatt
Attachment #258662 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #258673 - Flags: first-review?(michael.buettner)
Comment on attachment 258673 [details] [diff] [review]
fixes the actual problem with the datetextpicker

>+               // Figure out what day of the week today is.
>+               var today = createDateTime();
>+               today.jsDate = new Date();
>+               today.getInTimezone(calendarDefaultTimezone());
You probably would want to write
                 var today = jsDateToDateTime((new Date())).getInTimezone(calendarDefaultTimezone());
But that's up to you, I'm fine with both variants. I just find the latter more readable.

>+               // i-weekday gets the offset. Add 7 to ensure that the %
>+               // operation stays positive.
>+               var offset = (i - today.weekday + 7) % 7;
>+               today.day = today.day + offset;
>+               today.normalize();
>+               return today.jsDate;
Besides the fact that this stuff will horribly fail in case the UTC-offset from calendarDefaultTimezone() and from the OS timezone differ, it looks good to me. Since this is just one more spot that suffers from this problem, I think we should just go ahead with it. r=mickey.
Attachment #258673 - Flags: first-review?(michael.buettner) → first-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
verified on lightning 0.5 RC1 
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.5+
You need to log in before you can comment on or make changes to this bug.