Skip to content
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

[12.0.0+] Incorrect date format conversion on input #9681

Closed
adrianszymanski89 opened this issue Jul 18, 2022 · 14 comments · Fixed by #9878
Closed

[12.0.0+] Incorrect date format conversion on input #9681

adrianszymanski89 opened this issue Jul 18, 2022 · 14 comments · Fixed by #9878
Assignees
Labels
bug Cell type: Date Regression Issues that were created while adding new changes to the source code Status: Released

Comments

@adrianszymanski89
Copy link
Contributor

Description

When we have dateFromat set to MM/DD/YY and we input, for example - 7/1/22 in the cell, it is incorrectly converted to 01/07/2022. It used to be converted correctly in Handsontable v. 11.1.0 (before updating moment.js dependency #9381)

Steps to reproduce

  1. Type 7/1/22 in the date cell;
  2. Press enter;

Demo

V 11.1.0 (works correctly) - https://jsfiddle.net/aszymanski/nqv3btc0/1/
V 12.0.0 (and higher, it's broken) - https://jsfiddle.net/aszymanski/nqv3btc0/2/

Your environment

  • Handsontable version: 12.0.0+
  • Browser name and version: Google Chrome 103
  • Operating system: macOS Monterey 12.4
@adrianszymanski89
Copy link
Contributor Author

Inform ZD (29687)

@adrianszymanski89 adrianszymanski89 changed the title [12.0.0+] Incorrect data format conversion on input [12.0.0+] Incorrect date format conversion on input Jul 18, 2022
@aninde aninde added the Regression Issues that were created while adding new changes to the source code label Jul 18, 2022
@aninde
Copy link
Contributor

aninde commented Jul 18, 2022

Another example for this case
date format: 'YYYY-MM-DD'
Typing 10-11-12, I expected 2010-11-12, but the result is inverted to 2012-11-10.

Screen.Recording.2022-07-18.at.15.59.33.mov

@wszymanski
Copy link
Contributor

wszymanski commented Jul 19, 2022

What is the goal? If you want only one digit of the date to be displayed (if you are dealing with a single digit number) please use appropriate M/D/YYYY format. Please let me know if that's what you mean.

edit: Okey, it's probably about correctFormat. Please provide information about it in the issue description and additionally show an expected result.

I've checked the code and it seems that:

a) default dateFormat also hasn't been changed.
b) used correctFormat method hasn't been changed.

/**
* Format the given string using moment.js' format feature
*
* @param {String} value
* @param {String} dateFormat
* @returns {String}
*/
let correctFormat = function correctFormat(value, dateFormat) {
let date = moment(new Date(value));
// Ugly fix for moment bug which can not format 5-digits year using YYYY
if (date.format('YYYY').length > 4) {
date.year((date.year() + '').substr(0, 4));
}
return date.format(dateFormat);
};

Upgraded moment.js may have bug or library API has changed. We upgraded from 2.24.0 to 2.29.3 (12.0.0 release) within #9382 and from 2.29.3 to 2.29.4 (12.1.2 release) within #9637.

@warpech
Copy link
Member

warpech commented Jul 19, 2022

Okey, it's probably about correctFormat. Please provide information about it in the issue description and additionally show an expected result.

I don't think so. I can see the same problem without correctFormat:

I think the problem is that Pikaday transforms the input before closing the editor. On this line, Pikaday chooses defaultDateFormat:

options.format = options.format || this.defaultDateFormat;

Why is this a problem starting from 12.0, I don't know. Maybe something has changed in the editor closing code.

@wszymanski
Copy link
Contributor

Okey, it's probably about correctFormat. Please provide information about it in the issue description and additionally show an expected result.

I don't think so. I can see the same problem without correctFormat:

I think the problem is that Pikaday transforms the input before closing the editor. On this line, Pikaday chooses defaultDateFormat:

options.format = options.format || this.defaultDateFormat;

Why is this a problem starting from 12.0, I don't know. Maybe something has changed in the editor closing code.

I'll check that.

@wszymanski
Copy link
Contributor

wszymanski commented Jul 19, 2022

Okey, it's probably about correctFormat. Please provide information about it in the issue description and additionally show an expected result.

I don't think so. I can see the same problem without correctFormat:

True.

I think the problem is that Pikaday transforms the input before closing the editor.

The below Pikaday's code changes how date look like after closing the editor. Maybe it's related to introducing ShortcutManager. Maybe some event isn't prevented. I'll check that yet.

https://github.com/Pikaday/Pikaday/blob/21f676e70d688d18b265f2c12fc38e8457c20645/pikaday.js#L595

@wszymanski
Copy link
Contributor

I confirm that after checking develop branch I can see that c06c32b introduce changes which affects in entering to mentioned above part of the code. On the previous e8e3a9f commit Pikaday's setDate method isn't called.

@wszymanski
Copy link
Contributor

wszymanski commented Jul 20, 2022

@adrianszymanski89 @warpech I've created a workaround, but I assume that fix should be provided in a different place. It corrects incorrect behavior of using Pikaday's method, not the cause of the problem.

Workaround: https://jsfiddle.net/pqn910ju/

@krzysztofspilka krzysztofspilka added this to the 12.2 (September 2022) milestone Aug 18, 2022
@wszymanski wszymanski self-assigned this Sep 1, 2022
wszymanski added a commit that referenced this issue Sep 6, 2022
@wszymanski
Copy link
Contributor

wszymanski commented Sep 6, 2022

It seems that there are two bugs:

  1. The value inside the cell is changed right after pressing ENTER key (from 01/07/2022 to 07/01/2022).
Sep-06-2022.17-16-38.mp4
  1. The value shown in the widget is wrong (it shows 7th of January 2022 instead of 1st of July 2022).

Screenshot 2022-09-06 at 17 14 01

The first problem has been fixed by commit: 824ab4e. The window object is capturing an event later that document.documentElement (it occurs as next in the hierarchy). That's why Pikaday has been catching the change event as first (before keydown event). This leads us to some problems.

The second problem is investigated. Pikaday uses moment internally and call toDate method which doesn't work as I assume (please see the below example).

const momentDate = moment('01/07/2022', 'MM/DD/YYYY');
momentDate.toDate(); // Fri Jan 07 2022 00:00:00 GMT+0100 (Central European Standard Time)

edit: It seems that method works properly.

@wszymanski
Copy link
Contributor

I've found extra problem related to flickering while entering data by copy+paste and enter key.

flickering.mp4

I'll restore mechanism which has been working up to version 12.0.0 (an extra event.stopPropagation blocked event captured by Pikaday).

@budnix
Copy link
Member

budnix commented Sep 12, 2022

@wszymanski, I believe you closed the issue by accident 😄 The issue should be closed by the support team after the fix is released and confirmed that it works.

@wszymanski
Copy link
Contributor

@wszymanski, I believe you closed the issue by accident 😄 The issue should be closed by the support team after the fix is released and confirmed that it works.

Sure, thanks. It has been closed automatically, after merging the PR. It probably happened after using the word "fixed".

@budnix budnix reopened this Sep 16, 2022
@aninde
Copy link
Contributor

aninde commented Sep 22, 2022

This issue will be fixed by v12.1.3

@AMBudnik
Copy link
Contributor

Issue closed as fixed in Handsontable v12.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Cell type: Date Regression Issues that were created while adding new changes to the source code Status: Released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants