Closed Bug 1396639 Opened 7 years ago Closed 6 years ago

Start time and end time are modified for an event

Categories

(Calendar :: Lightning Only, defect)

Product:
Calendar
Component:
Lightning Only
Version:
Lightning 5.8
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: GeekShadow, Assigned: merike)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Dricks test with alerts
200.96 KB, image/png
Details
timeshift_v2
2.53 KB, patch
merike
: review+
Details | Diff | Splinter Review 
When you create an event and set the start/end time, it gets changed when you focus out.

Steps to reproduce :
1. Open Calendar view
2. Click on Event
3. Set the start time at 14:00
4. Click on end time
5. The start time is set to 13:09 instead of keeping 14:00
6. The same applies for the end time

Tested with Thunderbird 56.0b3 (64 bits) and Lightning 5.8b1
Same issue with Lightning 5.8b4
Same problem here with Thunderbird 56b3 and Lightning 5.8b3
If you ever click on the ">>" in the time picker to show minutes by minutes, you will trigger this bug "forever".

The problem comes from datetimepickers.xml

The method "formatTime" has been changed to:
<method name="formatTime">
<parameter name="aValue"/>
<body><![CDATA[
let formatter = Services.intl.createDateTimeFormat(undefined, this.kTimeFormatObject);
return formatter.format(aValue);
]]></body>
</method>

but formatter.format(aValue) returns an incorrect value.
Is the TB and Lightning version for which the issue is observed a vanilla version from Mozilla or from a package provided by the distribution? Which distribution do you use?
Flags: needinfo?(antoine.mozilla)
(In reply to [:MakeMyDay] from comment #3)
> Is the TB and Lightning version for which the issue is observed a vanilla
> version from Mozilla or from a package provided by the distribution? Which
> distribution do you use?

It's from a vanilla version from Mozilla (beta channel) using Ubuntu 16.04.3 LTS in both french locale.
Flags: needinfo?(antoine.mozilla)
Wayne, do you have somebody on your list who's on Linux but not Kubuntu 16.04 we could reach out to try to reproduce this issue on Beta or Daily? Both reports we have so far are referring to that distribution.
Flags: needinfo?(vseerror)
(In reply to [:MakeMyDay] from comment #6)
> Wayne, do you have somebody on your list who's on Linux but not Kubuntu
> 16.04 we could reach out to try to reproduce this issue on Beta or Daily?
> Both reports we have so far are referring to that distribution.

I have Chris as being on LMDE
Flags: needinfo?(vseerror) → needinfo?(chris.ramsden)
I can't replicate this. 

Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Thunderbird/56.0 (b3 actually)
Lightning 5.8b4
Linux 4.7.0-0.bpo.1-amd64 x86_64
MX Linux 16 (Metamorphosis) with Mate 1.8.1 DE. 

Thunderbird and lightning downloaded vanilla from mozilla.org site. 

The start time stays as set, i.e. 14:00 for the example given. 

The end time /does/ change as the start time is changed, but it is simply honouring my default 60 minute duration for events. Minute-by-minute adjustment using the '>>' is fine too.  

I have the chain link to tie the start and end times together UNchecked.
(In reply to Antoine Turmel [:GeekShadow] from comment #4)

> It's from a vanilla version from Mozilla (beta channel) using Ubuntu 16.04.3
> LTS in both french locale.

I'm using the Windows version, on French locale

Maybe this problem arise because of the locale ?
Chris, thanks for testing. Did you use a localized build or en-us?

@Dricks, @Antoine: can you reproduce this with any extension but Lightning disabled? It's likely that some addons didnn't yet adopt the datetime formatting changes. If it's still an issue in that setup, can you try to reproduce it on your system with an en-us beta version of TB and Lightning? What is your setting of the pref intl.regional_prefs.use_os_locales and does flipping it (or setting it respectively in the pref UI) make any difference for you?
Flags: needinfo?(dricks2222)
Flags: needinfo?(antoine.mozilla)
I'm en-gb.
Flags: needinfo?(chris.ramsden)
I'm not using any extension but lightning.
Same problem using Thunderbird 56b4 en-us and lightning 5.8.4 build14 en-us

The pref intl.regional_prefs.use_os_locales is false, switching to true doesn't solve the problem.

Again, the problem comes from 
let formatter = Services.intl.createDateTimeFormat(undefined, this.kTimeFormatObject);
return formatter.format(aValue);

I'll put some alerts tomorrow and post the results as screenshots with the provided hours above
Flags: needinfo?(dricks2222)
Attached image Dricks test with alerts — 
Dricks test with alerts
14:00 translate to 13:09
Code to show alerts :
<method name="formatTime">
<parameter name="aValue"/>
<body><![CDATA[
let formatter = Services.intl.createDateTimeFormat(undefined, this.kTimeFormatObject);
let valueFormatted = formatter.format(aValue);
fidUtils.alert("formatTime : " + aValue + " - " + formatter + " - " + valueFormatted);
return valueFormatted;
]]></body>
</method>


Results (cf screenshots) :
formatTime : Sun Dec 31 1899 14:00:00 GMT+0100 - [object Object] - 13:09
             |___________1___________________|   |_____2_______|   |_3__|

1: aValue
2: formatter
3: valueFormatted

So, it calls
formatter(Sun Dec 31 1899 14:00:00 GMT+0100) which returns  "13:09"
Which replace 14:00 as selected hour with 13:09
Keywords: regression
Dricks, thanks for trying to debug the issue. Unfortunately, I cannot reproduce the issue here, so can you please add the following debug code to the method formatTime you already added your code to:

cal.LOG(this.kTimeFormatObject);
cal.LOG(Services.intl.createDateTimeFormat(undefined, { timeStyle: "short" }).format(new Date(1899, 11, 31, 14, 0, 0))));
cal.LOG(Services.intl.createDateTimeFormat(undefined, { timeStyle: "short" }).format(new Date(2017, 11, 31, 14, 0, 0)));

You would need to enable calendar.debug.log in TB advanced configuration before reproducing the issue. Please post the results you get in the error console for all three debug entries here.
Flags: needinfo?(dricks2222)
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
I get:

Logging object...
End object

12:39
14:00

Any year before 1921 seems to get minutes other than 0 with 31st Dec, no clue why 1921 and forward does not.. Hours vary, assuming it's DST-related?
My guess would be that the new intl library correctly uses historic timezone information. 

If you go to https://www.timeanddate.com/time/zone/france/paris and look at the timezones changes in 1850-1899 and 1900-1924 you see that this timezone used UTC+0:09:21 until 1911. So a time from today using UTC+1:00:00 mapped to 1899 using UTC+0:09:21 would explain the change from 14:00 to 13:09.

Assuming that Merike is using timezone like https://www.timeanddate.com/time/zone/estonia/tallinn you can see that this timezone used UTC +1:39 in 1899, and UTC +0:21 in 1921 before switching to UTC +2:00 in 1922.

Solution might be to not use a date in 1899 to format the times or to somehow enforce usage of current timezone information.
Or maybe the absence of a date information when formatting a time is interpreted as 1900-01-01 / 1899-12-31 causing the historic timezone information from this year to be applied.

Is it possible to reproduce the issue in Firefox? If yes this could be a toolkit problem. If not this might a problem with Lightning using the intl library.
Merike, thanks for checking. What is your OS configured date format? I you should get the same behaviour in FF with the 1899 date when applying toLocaleTimeString(undefined, { timeStyle: "short" }) on the JS Date object.

Without having looked at the code right now, I assume this is not a bug in the time formatting itself but (mis)interpretation of the respective date. We have test code in the picker based on year 2000 (though I don't know why there is a 100 year offset) which might trigger a conversion to 1900 with certain OS datetime settings (which might be a bug in the intl implementation or a shortcoming due to short year formatting, for which you would have to specify the year range to apply the 2 digit years) and apply correctly the then current tz offset.

If this is it, it should be safe to use the current date for such a check if it is still needed at all, since we could detect the am/pm usage simply by use of resolvedOptions or don't need this anymore since formatting is applied locale aware automatically now.
Can check later for Linux but on a Win10 machine with Firefox beta (et)

new Date(1899,11,31, 14,0,0).toLocaleTimeString(undefined, {timeStyle:"short"})
"13:39:00"

So you're definitely not safe in the territory of using such year for time formats. Dates are configured to show in Estonian formats on this machine, in Settings app short time is "15:52" (H:mm) currently for example.
> Solution might be to not use a date in 1899 to format the times or to somehow enforce usage of current timezone information.

We are periodically updating our timezone database and backport it to other branches.

I believe the last update was to tz2017c in bug 1411957.

If this data is wrong, we should report it to the maintainers?
Attached patch timeshift (obsolete) — — Splinter Review 
I don't see any unwanted side effects currently and it does fix the bug for me.
Assignee: nobody → merikes.lists
Status: NEW → ASSIGNED
Attachment #8935528 - Flags: review?(makemyday)
Comment on attachment 8935528 [details] [diff] [review]
timeshift

Review of attachment 8935528 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for picking this up.

The fixes your current issue, but leaves a gap, if there has been a timezone offset change (other then a DST change) earlier in a year, like for VET in May 2016. To fully eleminate this, we would always need the correct date context in parseTime, but without that, using also the current month and day is the best fit.

So, please add the current month and day. r+ with that fixed.
Attachment #8935528 - Flags: review?(makemyday) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> > Solution might be to not use a date in 1899 to format the times or to somehow enforce usage of current timezone information.
> 
> We are periodically updating our timezone database and backport it to other
> branches.
> 
> I believe the last update was to tz2017c in bug 1411957.
> 
> If this data is wrong, we should report it to the maintainers?

2017c is the most recent version of the tz definition, so we're fine with that. The odd looking offset comes from using local mean time for times before the standard time system was introduced for a respective area in tz db, which is imho the correct thing to do - so there's no need for an upstream report.
Flags: needinfo?(dricks2222)
Flags: needinfo?(antoine.mozilla)
Attached patch timeshift_v2 — — Splinter Review 

        Attachment #8935528 -
        Attachment is obsolete: true

        Attachment #8936055 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Any objections to not evaluating now.getXXX() multiple times? That is:
let now = new Date();
let curYear = now.getFullYear();
let cutMonth = ... etc.
(Just asking.)

    
    

    
  
AFAICT there are 3 possible cases, you typed noon, midnight or a general time. In each case each now.getXXX() call is evaluated only once. You cannot have a noon and a midnight at the same time for example. Therefore it didn't make much sense to me to have 3 extra variables for storing the calls.

    
    

    
  
Thanks, I'll get it landed.

    
    

    
  
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5534aed3ebb2
fix undesired modification of start time and end time for an event. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

    
    

    
  
I've tweaked the commit message to reflect what was fixed.
Target Milestone: --- → 6.1

    
    

    
  
Thanks, I have patched a 6.0b2 version for my Thunderbird in beta channel, and it's working great :)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: