New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

October broken on Firefox Nightly #18

Closed
fczuardi opened this Issue Apr 29, 2015 · 19 comments

Comments

2 participants
@fczuardi
Contributor

fczuardi commented Apr 29, 2015

Steps to reproduce:

  1. Download and install Firefox Nightly 40.0a1 (2015-04-29) (mac osx 10.9.5)
  2. Open the demo page at http://www.gpbl.org/react-day-picker/
  3. Navigate to October 2014, or October 2015 or October 2016

Expected results:

  • A nice looking calendar for that month

Actual results:

  • The 4th line duplicates the 3th line and then extends 6 more days missing one, example (October 2014): 12, 13, 14, 15, 16, 17, 18, 20, 21, 22, 23, 24, 25

screen shot 2015-04-29 at 9 38 47 am

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

This is a warning I get on the console when I browse to one of the broken Octobers:

"Warning: flattenChildren(...): Encountered two children with the same key,.$290. Child keys must be unique; when two children share a key, only the first child will be used."

Contributor

fczuardi commented Apr 29, 2015

This is a warning I get on the console when I browse to one of the broken Octobers:

"Warning: flattenChildren(...): Encountered two children with the same key,.$290. Child keys must be unique; when two children share a key, only the first child will be used."

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

I am trying to dig into this, the function weeks of ./CalendarUtils is returning one 8 days week for the month of october for some reason:

Array [ Array[3], Array[7], Array[8], Array[7], Array[7] ]
Contributor

fczuardi commented Apr 29, 2015

I am trying to dig into this, the function weeks of ./CalendarUtils is returning one 8 days week for the month of october for some reason:

Array [ Array[3], Array[7], Array[8], Array[7], Array[7] ]
@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

I wonder if this might be a daylight saving time issue, patching this CalendarUtils.js line

https://github.com/gpbl/react-day-picker/blob/master/dist/CalendarUtils.js#L22

to increase the day using:

currentDay.endOf('day').add(1, 'd');

instead of

currentDay.add(1, 'd');

fixed the "8 days week" issue (and duplicated day in a week, and duplicate key in react issues), but not the layout of the calendar yet, maybe because of a similar addition or subtractions of day in the code.

I will dig deeper and look for a related moments issue as well…

Contributor

fczuardi commented Apr 29, 2015

I wonder if this might be a daylight saving time issue, patching this CalendarUtils.js line

https://github.com/gpbl/react-day-picker/blob/master/dist/CalendarUtils.js#L22

to increase the day using:

currentDay.endOf('day').add(1, 'd');

instead of

currentDay.add(1, 'd');

fixed the "8 days week" issue (and duplicated day in a week, and duplicate key in react issues), but not the layout of the calendar yet, maybe because of a similar addition or subtractions of day in the code.

I will dig deeper and look for a related moments issue as well…

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

A simplified testcase:

var oct182014Plus1 = moment('2014-10-18').add(1,'d');
console.log('18 of october 2014 + 1 day', oct182014Plus1.format(), oct182014Plus1.valueOf());

On Chrome returns:

18 of october 2014 + 1 day 2014-10-19T01:00:00-02:00 1413687600000

and on Firefox:

"18 of october 2014 + 1 day" "2014-10-18T23:00:00-03:00" 1413684000000
Contributor

fczuardi commented Apr 29, 2015

A simplified testcase:

var oct182014Plus1 = moment('2014-10-18').add(1,'d');
console.log('18 of october 2014 + 1 day', oct182014Plus1.format(), oct182014Plus1.valueOf());

On Chrome returns:

18 of october 2014 + 1 day 2014-10-19T01:00:00-02:00 1413687600000

and on Firefox:

"18 of october 2014 + 1 day" "2014-10-18T23:00:00-03:00" 1413684000000
@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

Ok, it boils down to local date vs utc date, one possible fix is to update line #22 of CalendarUtils.js to use UTC instead of local, with:

      currentDay.utc().add(1, 'd');
Contributor

fczuardi commented Apr 29, 2015

Ok, it boils down to local date vs utc date, one possible fix is to update line #22 of CalendarUtils.js to use UTC instead of local, with:

      currentDay.utc().add(1, 'd');

fczuardi added a commit to fczuardi/react-day-picker that referenced this issue Apr 29, 2015

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

I have created a pull request with a fix, please let me know if this is not the proper fix or the proper way to contribute :)

Contributor

fczuardi commented Apr 29, 2015

I have created a pull request with a fix, please let me know if this is not the proper fix or the proper way to contribute :)

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Apr 29, 2015

Owner

@fczuardi thanks for taking care, it is indeed a weird issue. i'd like to investigate it first – maybe a problem with the nightly itself?

Owner

gpbl commented Apr 29, 2015

@fczuardi thanks for taking care, it is indeed a weird issue. i'd like to investigate it first – maybe a problem with the nightly itself?

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi Apr 29, 2015

Contributor

Yes, it might be a firefox bug, please leave an update in the related moment js issue if you figure out.

Contributor

fczuardi commented Apr 29, 2015

Yes, it might be a firefox bug, please leave an update in the related moment js issue if you figure out.

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi May 14, 2015

Contributor

@gpbl check the comments on moment/moment#2353 to see the explanation, it is not necessarily a firefox bug since "The ECMAScript spec does not define the behavior for a local time value that falls into a DST transition", just a different implementation.

Anyways, from the moment.js folks this issue is a wontfix, they are fine living with october 18 + 1 day = october 18 since in brasilia's local time that day has 25 hours :)

As for the react-day-picker side of things, this is still a bug since it breaks the calendar layout.

Contributor

fczuardi commented May 14, 2015

@gpbl check the comments on moment/moment#2353 to see the explanation, it is not necessarily a firefox bug since "The ECMAScript spec does not define the behavior for a local time value that falls into a DST transition", just a different implementation.

Anyways, from the moment.js folks this issue is a wontfix, they are fine living with october 18 + 1 day = october 18 since in brasilia's local time that day has 25 hours :)

As for the react-day-picker side of things, this is still a bug since it breaks the calendar layout.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 14, 2015

Owner

Hey @fczuardi thanks for the follow up. Interesting case. Will merge your PR 👍

Owner

gpbl commented May 14, 2015

Hey @fczuardi thanks for the follow up. Interesting case. Will merge your PR 👍

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 14, 2015

Owner

Fix published on npm as v0.8.2

Owner

gpbl commented May 14, 2015

Fix published on npm as v0.8.2

@gpbl gpbl closed this May 14, 2015

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi May 14, 2015

Contributor

Checking on the github commits historty doesn't look like the pr was merged, I will test it later on my computer to let you know if this is still happening in 0.8.2

Contributor

fczuardi commented May 14, 2015

Checking on the github commits historty doesn't look like the pr was merged, I will test it later on my computer to let you know if this is still happening in 0.8.2

gpbl added a commit that referenced this issue May 14, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 14, 2015

Owner

😱 sorry i made all wrong! republishing asap :)

Owner

gpbl commented May 14, 2015

😱 sorry i made all wrong! republishing asap :)

@gpbl gpbl reopened this May 14, 2015

@fczuardi

This comment has been minimized.

Show comment
Hide comment
@fczuardi

fczuardi May 14, 2015

Contributor

No worries, thanks for the useful component :)

Contributor

fczuardi commented May 14, 2015

No worries, thanks for the useful component :)

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 14, 2015

Owner

v0.8.3 💫

Owner

gpbl commented May 14, 2015

v0.8.3 💫

@gpbl gpbl closed this May 14, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 24, 2015

Owner

@fczuardi I am not yet happy with the fix :-) if we switch to UTC the dates clicked will be passed to the events as UTC as well. Developers doing date comparisons need to be aware about this and always work with UTC dates, which is unfair.

For version 1, I'm planning to move away from moment.js (#2): I need to build a small util for date calculations, which eventually should consider this edge case (after all, it happens only in a nightly build, right?). When the test suite is complete we can work better on this.

Owner

gpbl commented May 24, 2015

@fczuardi I am not yet happy with the fix :-) if we switch to UTC the dates clicked will be passed to the events as UTC as well. Developers doing date comparisons need to be aware about this and always work with UTC dates, which is unfair.

For version 1, I'm planning to move away from moment.js (#2): I need to build a small util for date calculations, which eventually should consider this edge case (after all, it happens only in a nightly build, right?). When the test suite is complete we can work better on this.

@gpbl gpbl reopened this May 24, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 24, 2015

Owner

For example, if not setting utc() ca7f1a3 this comparison would not work: https://github.com/gpbl/react-day-picker/blob/master/example/src/Utils.js#L15 (not sure why actually 😒)

Owner

gpbl commented May 24, 2015

For example, if not setting utc() ca7f1a3 this comparison would not work: https://github.com/gpbl/react-day-picker/blob/master/example/src/Utils.js#L15 (not sure why actually 😒)

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl May 31, 2015

Owner

I've moved back to the no-utc date, when closing #27 I'll keep an eye on this.

Owner

gpbl commented May 31, 2015

I've moved back to the no-utc date, when closing #27 I'll keep an eye on this.

@gpbl gpbl added this to the v1.0.0 milestone May 31, 2015

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jun 24, 2015

Owner

Should it be fixed with v1 😊

Owner

gpbl commented Jun 24, 2015

Should it be fixed with v1 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment