Closed Bug 366139 Opened 13 years ago Closed 9 years ago

Wrong end time set on multi-day events

Categories

(Calendar :: Dialogs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: bv1578)

References

Details

(Whiteboard: [needed beta][has l10n impact])

Attachments

(7 files, 3 obsolete files)

If you create a new event that lasts for more than a day and you set the end time before the start time, a wrong end date is used for the event.

Steps to reproduce: 
1. Open the "New event" dialog
2. Set a title (e.g. "Test")
3. Set the end date to tomorrow
4. Set the end time to 02:00
5. Set the start time to 21:00
6. Click OK

Expected results:
A new event from today, 21:00 until tomorrow, 02:00 is created.

Actual Results:
A new event from today, 21:00 until tomorrow, 23:00 is created.

Reproducible:
Always
Actually I think this works as intended: When changing the start date/time the event duration is kept the same and the end date/time is automatically updated. The recommended workflow is to set the start date/time before the end date/time.
In my case I knew the end time, but could not remember the start time. I had to look it up in my papers. So I entered the end time first. I agree that this might not be the normal workflow, but it may happen at times. 

After you explained the reasons, the change makes sense. The problem is, that the change is not discoverable for the user. The last thing you enter is the start date and you leave the cursor in the start date field. Then you immediately click on the ok button. The end date never changed in the dialog, so you think that everything is ok. But you get a wrong event.
(In reply to comment #2)
> The end date never changed in the dialog,
> so you think that everything is ok. But you get a wrong event.
> 
The end date does change in the dialog. Nominating for INVALID.
Whiteboard: [qa discussion needed]
Mickey,

What do you think about this?  I don't think it is worth changing our default behavior for, but it isn't very obvious what happens if the user changes the end time first.  I've done that once or twice myself.  Granted, this is one of those things that people will only do once or twice then they will learn "how to do it right" so to speak.

But, is there some way we can detect this situation (user edits end time first) and prevent a bad user experience in the first place?  Maybe just notify them that if they now edit the start, the end time will move.  

Is that worth the trouble?

OS: Linux → All
Hardware: PC → All
Version: Lightning 0.3 → unspecified
Whiteboard: [qa discussion needed]
I guess what we can do is set a "dirty" flag on the end time that gets reset each time the item is saved. When the user changes the end time then changing the start time wont further modify the end time. This shouldnt be too hard to solve.

Stefan, Sebastian, Patrick, what do you think? Does this make sense for the average user?
Whiteboard: [feedback?]
Flags: blocking-calendar1.0+
Whiteboard: [feedback?] → [not needed beta][no l10n impact][feedback?]
Whiteboard: [not needed beta][no l10n impact][feedback?] → [needed beta][no l10n impact][feedback?]
Attached image Screenshot - v1
Alternative suggestion. We could also make the time picker a bit shorter so there's more room for the timezone link.
Attachment #467777 - Flags: ui-review?(clarkbw)
Attachment #467777 - Attachment is patch: false
Attachment #467777 - Attachment mime type: text/plain → image/jpeg
I've tried to make something as proposed in the screenshot - v1 even if it hasn't had the UI review yet, so a different workflow can be tested.
The positive behavior is that, once start and end date are "linked", you can set a date-time or the other, without distinction, at any time without any warning about an end date that occurs before the start date.

I will ask for review after a possible positive UI review, but actually something different should be done about the button positioning and for the look (see the "disabled" icon in the next screenshot). The button is well placed on Win XP for Luna and Classic themes, but when the font size is bigger, the button is shifted to the top. I need some suggestion about a different way to place it because of the grid (I don't know a different way to place elements with a straddle position on two rows different from this one).

The icon on the button is copied from a GIMP's dialog button, hence I'm not sure if there is a license problem or other, but IMHO the vertical placement is better than the horizontal one because of the timezone links.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Component: General → Dialogs
QA Contact: general → dialogs
Whiteboard: [needed beta][no l10n impact][feedback?] → [needed beta][no l10n impact][needs ui-r]
adding andreas who's going to take over the review here
Attachment #467777 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
I think there might occur licensing issues here, since GIMP is licensed under GPL(v3 or later) Only and all the Mozilla code is tri- GPL,LGPL & MPL.
I can fix up some new graphics though.
I've been going back and forth on if it would be possible to do this without adding any more UI, but the ones I can think of could make some user cases tricky.
Personally I think we should add the chain-type icon, users know what this means and can easily see what the behavior is. Andreas, I'd appreciate if you could create a new icon for us!
Does this make it my patch and can I still review it?
Anyhow, I feel more and more like this is the way to go and would get some other issues out of the way as well.
Attachment #476795 - Flags: ui-review+
Comment on attachment 467777 [details]
Screenshot - v1

We're going to use the later patch, so I'm minusing this so no confusion about what have been reviewed gets done.
Attachment #467777 - Flags: ui-review?(nisses.mail) → ui-review-
I gave the patch an ui-r+, as I figured it was pretty much the same anyway.
(In reply to comment #15)
> I gave the patch an ui-r+, as I figured it was pretty much the same anyway.

I have to do a new patch because this one doesn't work well with all day events (because of bug 594403).
About the icon, what do I have to do? Do I have to add the last patch's icon with the graphic fix or waiting for a new icon?
Here is the graphics as a separate file.
You can use the graphics I just posted.
Attached patch patch - v2 (obsolete) — Splinter Review
This patch should work fine with all-day events too.
I hope everything is fine, in particular with the timezone associate to the dates.
The patch also fixes bug 594403 because it updates the gEndTime value inside the function dateTimeControls2State when the event is all-day, and this prevents the timezone link activation inside the function updateTimezone().
It remains the problem of the positioning for the button mentioned in comment #7, if you have a suggestion ...

The only issue is when the user wants to "undo" (CTRL+Z or menu Edit->Undo) a date just modified on any datepickers. In this case both datepickers might show two date-time not consistent with the state of the keep-duration button (or even wrong because e.g. start date is after the end date). This issue also occurs without the patch, because the undo command restores only the last changed datepicker and not the datepicker "linked" (by the code) with the former.
Not sure how this issue might be solved, doesn't seem existing any bug report about it.
Attachment #469432 - Attachment is obsolete: true
Attachment #477740 - Flags: review?(philipp)
Comment on attachment 477740 [details] [diff] [review]
patch - v2


We should add a tooltiptext to the link icon. Aside from newbie users understanding what the icon does, this also gives it an acessible name for users that cannot see the chain icon.

About the spacing issue of the icon, theres probably not much we can do here without breaking something else.

* One thing you could try is specifying the bottom margin in em units instead of px. I don't have a windows box to test, but a value of -1.8em was quite ok for Linux. This works for the default font size of 10, and for example 14. It still causes misalignment on some other font sizes though.

* This option could possibly break some horizontal alignment of the controls, but you could put the start/end date controls into one grid row and then use v/hboxes to fit the controls, then put the chain icon together with the line icons into a vbox that reaches across the whole height of the start/end date rows.

Since undo is probably a rather rare case, I'd suggest to file a bug and have that fixed some time later.


I've taken a look at the code and there's nothing to nag about :-) Let me know what you think, if you want to keep the patch the way it is then I'll fix the tooltiptext and check it in.
Attachment #477740 - Flags: review?(philipp) → review+
Whiteboard: [needed beta][no l10n impact][needs ui-r] → [needed beta][no l10n impact][needs checkin patch]
(In reply to comment #20)

> Let me know
> what you think, if you want to keep the patch the way it is then I'll fix the
> tooltiptext and check it in.

I'm going to try your suggestions about bottom margin and a different placement for the controls in the grid rows, although I had already seen that playing with the rows in that code always breaks something in the dialog.
Attached patch patch v3Splinter Review
(In reply to comment #20)

> * One thing you could try is specifying the bottom margin in em units instead
> of px. I don't have a windows box to test, but a value of -1.8em was quite ok
> for Linux. This works for the default font size of 10, and for example 14. It
> still causes misalignment on some other font sizes though.

With -1.8em I have 1 pixel misalignment with Win XP and system font Segoe Ui 8pt (my default font). Instead -1.9em would be better, but it is worse when font size increases (see the next screenshot).
In this new patch v3 I've changed a few things. The structure is the same but top image and the button are non longer linked. The behavior is better with bigger fonts in particular for the two images on top and bottom the button that are more centered (see the next screenshot). Could you give it a try on Linux?


> * This option could possibly break some horizontal alignment of the controls,
> but you could put the start/end date controls into one grid row and then use
> v/hboxes to fit the controls, then put the chain icon together with the line
> icons into a vbox that reaches across the whole height of the start/end date
> rows.

I've also made the patch in this way. In this case there isn't any problem with the alignment with big and small font, though it changes a lot the structure of dialog. I have a complete patch and I can attach it if you want, just let me know.


> We should add a tooltiptext to the link icon. Aside from newbie users
> understanding what the icon does, this also gives it an acessible name for
> users that cannot see the chain icon.

I've added a "-moz-user-focus: normal" to make the button activable with the tab key. (Why the other controls don't need it?).
Moreover I've put an accesskey and a tooltiptext in the button, but I'm not sure about the sentence, in particular any reference to the word "duration" doesn't seem fine to me, maybe something like "Keep the dates linked"?
Please Philipp write you something that explains the use of the button. I've temporary written a "k" as accesskey and an empty string for the entity event.dialog.keepDurationButton.tooltip inside the calendar-event-dialog.dtd file.
Attachment #482695 - Flags: review?(philipp)
Whiteboard: [needed beta][no l10n impact][needs checkin patch] → [needed beta][no l10n impact]
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/75c93fbd10fe>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/d2a98dbc129a>
a=philipp
Target Milestone: Trunk → 1.0b3
Comment on attachment 482695 [details] [diff] [review]
patch v3

Of course this patch has r=philipp, looks like I forgot to mark it.
Attachment #482695 - Flags: review?(philipp) → review+
Ahm, we still need a text for the tooltip, don't we?
And in comment #20 an accesskey was mentioned, but I don't see it in the patch. Still to come?
(In reply to comment #27)
> Ahm, we still need a text for the tooltip, don't we?
> And in comment #20 an accesskey was mentioned, but I don't see it in the patch.
> Still to come?

Ooops, sorry. It seems I attached the version previous to that one with the accesskey. I'm going to attach a patch in a few hours
Attached patch fix accesskey and tooltip text (obsolete) — Splinter Review
Accesskey: "k" key
Tooltip text: "Keep the dates linked", but, as I said in comment #22, please write you something better.
Attachment #477740 - Attachment is obsolete: true
Attachment #497271 - Flags: review?(philipp)
I'm reopening because of this test case:

- Make sure the chain link thing is closed/locked
- Create Event (17th of december)
- Select All Day
- Select the start date to be after the end date(27th december), the end date gets changed to an invalid value (22 december)
- Then "unlink" the dates and click on the start textbox, and you end up in a nasty popup hell (seems to popup twice each time).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Could the fix for bug 392448 be the cause for the double popup?
(In reply to comment #30)

> - Make sure the chain link thing is closed/locked
> - Create Event (17th of december)
> - Select All Day
> - Select the start date to be after the end date(27th december), the end date
> gets changed to an invalid value (22 december)
> - Then "unlink" the dates and click on the start textbox, and you end up in a
> nasty popup hell (seems to popup twice each time).

I can't reproduce the issue(s) with this test case on

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20101214 Lightning/1.0b3pre (BuildID=20101214035725) Lanikai/3.1.8pre

on an old and new profile.
Neither the invalid date nor the nasty popup appear in the test.
Bug 392448 seems fixed too.

But at the moment I can't test with Shredder. Does it happen with Lightning 1.1a1pre?
I could repro on 2 different computers:

WinXP w/ TB3.1.5 + Lightning nightly 1.0b3pre 14-Dec-2010 07:36
Win7 w/ TB3.1.6 + Lightning nightly 1.0b3pre 14-Dec-2010 07:36

Here is a more detailed howto repro:
- Create a local calendar
- Select week view, place it on the week of december 20
- Double click right under "december 20" to create a day event
- Make sure the chains are "linked" 
- Click on the down arrow next to the start date textbox to see the date picker
- Select "December 27"
- Observe that the end date is now "December 22" (Bug #1)
- Click on the chain so it becomes "unlinked"
- Click on the Start date date picker
- Observe 2 consecutive popups warning about the start date being higher than end date (and the date picker inaccessible) (Bug #2)
I closely repeated the steps to reproduce, but I can't definitely reproduce it.
When I select "December 27" on the Start date datepicker, the end date on the End datepicker becomes, correctly, "December 27". Obviously Bug #2 doesn't occur because somehow it's caused by the first one.

I'm going to look into the patch to see if there is some kind of critical situation.
Simon, just to clarify:
1) when you write

> - Double click right under "december 20" to create a day event

you mean "to create an *all day* event" (as you said in the previous comment) i.e. click in the header area, where there are the all day events, don't you?

2) I don't fully understand if the bug occurs only on the 27 of December (given the 20 of December as initial date) or it occurs with any date later than the 20 of December. By chance is it a couple of date with seven day difference like 20-27, 21-28 etc. that causes the bug?


Bug 392448 wouldn't seem related because the two lines of that patch are called only when the user closes, or saves and closes, the dialog.
If you want to try Lightning without the patch for bug 392448, I've uploaded  the 14/12/2010 nightly (Windows) with that patch commented out at this url:

http://www.mediafire.com/file/tfj0g4g52wsn217/lightning1.0b3pre-20101214-bug392448.xpi
> 2) I don't fully understand if the bug occurs only on the 27 of December (given
> the 20 of December as initial date) or it occurs with any date later than the
> 20 of December. By chance is it a couple of date with seven day difference like
> 20-27, 21-28 etc. that causes the bug?

I answer by myself, in the previous comment you talked of the 17-27 of December :-(
I tried the build without bug 392448 and I still see the bug. I will try to isolate the problem more, here is some more info:

No matter what date in the future I pick the end date will be 5 days before. 

So if the original start date and end date is 20th december and I pick the 21st, the end date becomes the 16th of december, If I pick the december 31, the end date becomes december 26...
Note that in order to reproduce you must have the Timezone Option not checked, from the event dialog: Options->Timezone. Maybe that is why you were not able to repro?
OK.
I have changed the timezone setting (in Tools->Options->Timezone) form the default value Europe/Berlin to America/New York and now I can finally reproduce the bug (I had started thinking it was time to buy a new computer).
I had some doubt about the timezone behavior as I said in comment #19 ;-).
Just curiosity, did you change the default timezone setting?

However, while working on this fix, I would like to see a decision about bug 619515 because if we want to restore the previous interface behavior, this bug must be solved in a different way.
1) I question the need for the link icon at all.  I agree with Philipp's suggestion in comment 5 to handle this internally with a flag instead of making users handle it.  That is, link the two dates until the first time the user changes the end date.

2) While that behavior is useful when first creating an event/task, I think it does more harm than good when editing an existing event/task.  For example when I edit an existing multiday event, 90% of the time I need to change either the start or end date, but Lightning changes both dates which is easy to miss and creates more work for me.  I believe that tasks are the same.  I'm more likely to change only the start or due date than both at once (except during initial creation).

Anyway since Lightning doesn't know what the user wants, IMO it shouldn't try to guess because it'll be wrong at least 50% of the time.

If you keep the link icon:

3) I suggest to not link dates together by default.  Let the user discover the link icon before you turn it on.

4) On my 1920 x 1200 resolution I didn't notice at first that the icon changes between two different states.  It would be more obvious if it changed to a different color or something.

5) When the two dates aren't linked, I receive an error message when I try to set a start date in the future before I've had a chance to set the end date.  Perhaps that error message should be postponed until I click 'save and close'.
I did an informal usability test around the office (~10 people, mix of technical and non-technical people) with the new dialog: 
- Most Lightning users did notice the new icon
- Most techies understood the feature right away
- Some thought it had something to do with recurring meetings
- Some thought it looked like a pair of glasses sideways
- General feedback was negative with most people not liking the feature or finding it a bit useless. Most said they liked the old behavior better where only the start time was linked all the time.
- An alternative was suggested by two people to add instead a checkbox in the date picker "keep duration" which they thought would make the feature more intuitive.

Major concerns about the default value of "linked" or "unlinked" was raised with no consensus since people like to have the start time linked but not the end time.
(In reply to comment #41)

> - Some thought it looked like a pair of glasses sideways

LOL! An advanced user I suppose ;-)

In this case, considered bug 619515 and comment #40 as well, I think it's better to restore the previous behavior and solve this bug in another way.
Status: REOPENED → ASSIGNED
(In reply to comment #42)

Does this mean the patches already checked in will be backed out soon?

This would be nice to know because then localizers could avoid redundant work (although it really isn't much).
Simon, Pete, Stefan,
I've done a different version that allows to bind (with the same button) only the End date to the Start date but not the vice-versa, and with a default "not-linked" status.
In this way we have by default the same behavior of the previous releases, but when the button is "linked", the End date changes the Start date in order to keep the same duration (the End date will have the same behavior that normally has the Start date), hence request in comment #2 is satisfied.

Could someone of you be so kind to give it a try? Obviously not for a review but only to test the workflow with this implementation (try it only on a new profile and without important data).
I've uploaded a build for Windows here:

http://www.mediafire.com/file/vxj1jpp7taa6w7g/lightning1.0b3pre-20101221-bug366139-Win.xpi

In any case I also agree if we want to back out the patch and find another solution.
Decathlon, thanks for the xpi.  In general I like the behavior.

However, I noticed if I change the start date, the end date, then the start date again, it changes the end date (the duration is kept).  Instead of that, another possible way is that the duration is never kept unless the button is enabled, then changing either date changes the other.  I don't really care though, I'd be happy with your latest patch.
Hi Decathlon, thanks for taking the time to rework this patch, I personally like the new behavior better and the arrow going from the end date to the start date to indicate the direction of the "link" is a nice touch. 

Pete: The behavior of start time modif => "Keep duration", end time modif => "Don't keep duration" is the current behavior and the behavior that seems to be the most popular and expected by users. Microsoft Outlook and Apple iCal have the same behavior.

With this latest patch we end up with a new feature: end time modif => "Don't keep duration" *OR* "keep duration". I would not mind the new feature to be merged but I think it should be associated with a new bug/ER. 

I think this bug should be marked as invalid since it essentially says that the behavior expected by most users is wrong.
Depends on: 619515
Comment on attachment 497271 [details] [diff] [review]
fix accesskey and tooltip text

This patch is fine, but given the previous comments I believe a new patch is planned? Do you need this checked in anyway, or would you like to make it part of the new patch? Do you want me to back out the previous patch?

I'm really impressed that you're still improving this patch, keep up the good work!
Attachment #497271 - Flags: review?(philipp) → review+
If we want to take this for 1.0b3, we should make sure the strings are checked in soon. There are currently 2 other bugs that require string changes, lets see if we can get them fixed first.
Whiteboard: [needed beta][no l10n impact] → [needed beta][has l10n impact]
Attached patch patch - v4Splinter Review
(In reply to comment #47)
> This patch is fine, but given the previous comments I believe a new patch is
> planned?

This patch is that one mentioned in comment #44, i.e the button links/unlinks only the *End* date to the *Start* date, that seems better received by the testers and doesn't causes bug 619515.

It doesn't need to back out the previous patch and it has the fix for the the access key and for the tooltip (Philipp, please write something different if you think).

It should fix the bug mentioned in comment #30 (Simon, could you please confirm that for the patch you tested?), moreover I've also added an arrow in the link-image on top of the button. This arrow appears/disappears when the button is linked/unlinked with the intent of further explain the meaning of the button i.e the End date causes/doesn't cause a change of the Start date. Along with the fixed arrow in the bottom link-image, the UI should give an information about the behavior of the datepickers and should make the UI (a bit) newbie proof, at least the button shouldn't be mistook for a pair of glasses sideways. ;-)

However, I repeat myself, I don't want to force the implementation of this patch. Given the others comments, I'm fine with backing out the patch already checked-in and solving the bug in another way. Philipp, it's up to you to take a decision.
Attachment #497271 - Attachment is obsolete: true
Attachment #502520 - Flags: review?(philipp)
Comment on attachment 502520 [details] [diff] [review]
patch - v4


>diff --git a/calendar/base/themes/common/images/event-dialog-keepduration-button.png b/calendar/base/themes/common/images/event-dialog-keepduration-button.png
This should be part of base/themes/$theme/dialogs/images/calendar-event-dialog.png

>+<!ENTITY event.dialog.keepDurationButton.tooltip   "Keep the duration when changing the End date">
>+<!ENTITY event.dialog.keepDurationButton.accesskey "k">
Text is fine with me :)

r=philipp codewise, I'd appreciate feedback from Simon.
Attachment #502520 - Flags: review?(philipp)
Attachment #502520 - Flags: review+
Attachment #502520 - Flags: feedback?(nomisvai)
Whiteboard: [needed beta][has l10n impact] → [needed beta][has l10n impact][needs feedback]
(In reply to comment #50)
> >+<!ENTITY event.dialog.keepDurationButton.tooltip   "Keep the duration when changing the End date">
Oh just a minor nit, lowercase E on End date. I'll fix this before checkin.
Comment on attachment 502520 [details] [diff] [review]
patch - v4

I've talked to simon via irc, he says the patch looks good. feedback+
Attachment #502520 - Flags: feedback?(nomisvai) → feedback+
Whiteboard: [needed beta][has l10n impact][needs feedback] → [needed beta][has l10n impact]
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/a2582159d295>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: 1.0b3 → Trunk
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/2d1f8c37944a>
a=philipp
Target Milestone: Trunk → 1.0b3
You need to log in before you can comment on or make changes to this bug.