351380 - wrong day selected after inserting a day name in the datetextpicker

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Andreas Treumann]

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

Comment 1

[Andreas Treumann]

Maybe a blocking bug for 0.3, because the datetextpicker is a new feature. 

Comment 2

[Joey Minta]

Probably related to timezones.  What timezone are you in and what time was it when you found this bug?

Comment 3

[Andreas Treumann]

My timezone is Europe/Berlin, and I also selected this timezone in Thunderbird at 'Tools/Options/Timezone.

Comment 4

[Andreas Treumann]

I found this bug yesterday at 4 pm. It was possible form me to repruduce this issue today at 9 am.

Comment 5

[Stefan Sitter [:ssitter]]

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.

Comment 6

[Joey Minta]

(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.

Comment 7

[Stefan Sitter [:ssitter]]

(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.

Comment 8

[Stefan Sitter [:ssitter]]

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.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

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)

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

(besides: it doesn't work at all for me. No matter what i type, it always ends up with today)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

It'd be nice to have, but we could live without it.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 13

[Michael Büttner (no reviews TFN)]

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.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: backout language parsing (checked in)

Patch disables the (never properly reviewed) attempt to do language parsing.

Comment 16

[Matthew (lilmatt) Willis]

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

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: remove entire UI

Alternative patch: remove the entire UI. Looks somewhat nicer.

Comment 18

[Joey Minta]

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.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 239017 [details] [diff] [review]
backout language parsing (checked in)

r2=dmose

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 23

[Matthew (lilmatt) Willis]

Comment on attachment 239030 [details] [diff] [review]
remove entire UI

Not going to use this approach

Comment 24

[Matthew (lilmatt) Willis]

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.

Comment 25

[Matthew (lilmatt) Willis]

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.

Comment 26

[Sebastian Schwieger]

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.

Comment 27

[Stefan Sitter [:ssitter]]

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.

Comment 28

[Matthew (lilmatt) Willis]

We should pull this for 0.5.

Comment 29

[Michael Büttner (no reviews TFN)]

I'd vote for removing the datetimepicker completely. Any objections?

Comment 30

[Matthew (lilmatt) Willis]

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.

Comment 31

[Michael Büttner (no reviews TFN)]

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.

Comment 32

[Matthew (lilmatt) Willis]

Attachment: Remove busted datepicker

Comment 33

[Michael Büttner (no reviews TFN)]

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

Comment 34

[Matthew (lilmatt) Willis]

Attachment: fixes the actual problem with the datetextpicker

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.

Comment 35

[Michael Büttner (no reviews TFN)]

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.

Comment 36

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 37

[Omar Bajraszewski]

verified on lightning 0.5 RC1