From 0fb4247ce834c475556a17e116e20f1ec0fd5a5a Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 3 Oct 2024 15:04:19 +0200 Subject: [PATCH] fix(material/core): avoid browser inconsistencies when parsing time Previously we were trying to rely on the browser to parse time, but we couldn't rely on it fully because browsers are inconsistent in how the handle time strings. We had some fallback logic to try and patch over it, but it led to some bug. These changes switch to relying fully on our own logic for parsing times. --- .../adapter/date-fns-adapter.spec.ts | 1 + .../adapter/luxon-date-adapter.spec.ts | 1 + .../core/datetime/native-date-adapter.spec.ts | 13 +-- .../core/datetime/native-date-adapter.ts | 99 +++++++++---------- 4 files changed, 50 insertions(+), 64 deletions(-) diff --git a/src/material-date-fns-adapter/adapter/date-fns-adapter.spec.ts b/src/material-date-fns-adapter/adapter/date-fns-adapter.spec.ts index f31457cbbce7..d85b32e8341d 100644 --- a/src/material-date-fns-adapter/adapter/date-fns-adapter.spec.ts +++ b/src/material-date-fns-adapter/adapter/date-fns-adapter.spec.ts @@ -546,6 +546,7 @@ describe('DateFnsAdapter', () => { expect(adapter.isValid(adapter.parseTime('24:05', 'p')!)).toBe(false); expect(adapter.isValid(adapter.parseTime('00:61:05', 'p')!)).toBe(false); expect(adapter.isValid(adapter.parseTime('14:52:78', 'p')!)).toBe(false); + expect(adapter.isValid(adapter.parseTime('12:10 PM11:10 PM', 'p')!)).toBe(false); }); it('should compare times', () => { diff --git a/src/material-luxon-adapter/adapter/luxon-date-adapter.spec.ts b/src/material-luxon-adapter/adapter/luxon-date-adapter.spec.ts index 4cbcf3655a37..12a4a95f81f3 100644 --- a/src/material-luxon-adapter/adapter/luxon-date-adapter.spec.ts +++ b/src/material-luxon-adapter/adapter/luxon-date-adapter.spec.ts @@ -648,6 +648,7 @@ describe('LuxonDateAdapter', () => { expect(adapter.isValid(adapter.parseTime('24:05', 't')!)).toBeFalse(); expect(adapter.isValid(adapter.parseTime('00:61:05', 'tt')!)).toBeFalse(); expect(adapter.isValid(adapter.parseTime('14:52:78', 'tt')!)).toBeFalse(); + expect(adapter.isValid(adapter.parseTime('12:10 PM11:10 PM', 'tt')!)).toBeFalse(); }); it('should return null when parsing unsupported time values', () => { diff --git a/src/material/core/datetime/native-date-adapter.spec.ts b/src/material/core/datetime/native-date-adapter.spec.ts index 5a2df4672f5b..a1264175bf02 100644 --- a/src/material/core/datetime/native-date-adapter.spec.ts +++ b/src/material/core/datetime/native-date-adapter.spec.ts @@ -1,18 +1,15 @@ import {LOCALE_ID} from '@angular/core'; import {TestBed} from '@angular/core/testing'; -import {Platform} from '@angular/cdk/platform'; import {DEC, FEB, JAN, MAR} from '../../testing'; import {DateAdapter, MAT_DATE_LOCALE, NativeDateAdapter, NativeDateModule} from './index'; describe('NativeDateAdapter', () => { let adapter: NativeDateAdapter; let assertValidDate: (d: Date | null, valid: boolean) => void; - let platform: Platform; beforeEach(() => { TestBed.configureTestingModule({imports: [NativeDateModule]}); adapter = TestBed.inject(DateAdapter) as NativeDateAdapter; - platform = TestBed.inject(Platform); assertValidDate = (d: Date | null, valid: boolean) => { expect(adapter.isDateInstance(d)) @@ -587,13 +584,9 @@ describe('NativeDateAdapter', () => { expect(adapter.isValid(adapter.parseTime('123')!)).toBe(false); expect(adapter.isValid(adapter.parseTime('14:52 PM')!)).toBe(false); expect(adapter.isValid(adapter.parseTime('24:05')!)).toBe(false); - - // Firefox is a bit more forgiving of invalid times than other browsers. - // E.g. these just roll over instead of producing an invalid object. - if (!platform.FIREFOX) { - expect(adapter.isValid(adapter.parseTime('00:61:05')!)).toBe(false); - expect(adapter.isValid(adapter.parseTime('14:52:78')!)).toBe(false); - } + expect(adapter.isValid(adapter.parseTime('00:61:05')!)).toBe(false); + expect(adapter.isValid(adapter.parseTime('14:52:78')!)).toBe(false); + expect(adapter.isValid(adapter.parseTime('12:10 PM11:10 PM')!)).toBe(false); }); it('should return null when parsing unsupported time values', () => { diff --git a/src/material/core/datetime/native-date-adapter.ts b/src/material/core/datetime/native-date-adapter.ts index dff89e74e6de..b4663da4dcef 100644 --- a/src/material/core/datetime/native-date-adapter.ts +++ b/src/material/core/datetime/native-date-adapter.ts @@ -28,7 +28,7 @@ const ISO_8601_REGEX = * - {{hours}}.{{minutes}} AM/PM * - {{hours}}.{{minutes}}.{{seconds}} AM/PM */ -const TIME_REGEX = /(\d?\d)[:.](\d?\d)(?:[:.](\d?\d))?\s*(AM|PM)?/i; +const TIME_REGEX = /^(\d?\d)[:.](\d?\d)(?:[:.](\d?\d))?\s*(AM|PM)?$/i; /** Creates an array and fills it with values. */ function range(length: number, valueFunction: (index: number) => T): T[] { @@ -292,67 +292,20 @@ export class NativeDateAdapter extends DateAdapter { return null; } - const today = this.today(); - const base = this.toIso8601(today); + // Attempt to parse the value directly. + let result = this._parseTimeString(value); - // JS is able to parse colon-separated times (including AM/PM) by - // appending it to a valid date string. Generate one from today's date. - let result = Date.parse(`${base} ${value}`); - - // Some locales use a dot instead of a colon as a separator, try replacing it before parsing. - if (!result && value.includes('.')) { - result = Date.parse(`${base} ${value.replace(/\./g, ':')}`); - } - - // Other locales add extra characters around the time, but are otherwise parseable + // Some locales add extra characters around the time, but are otherwise parseable // (e.g. `00:05 ч.` in bg-BG). Try replacing all non-number and non-colon characters. - if (!result) { + if (result === null) { const withoutExtras = value.replace(/[^0-9:(AM|PM)]/gi, '').trim(); if (withoutExtras.length > 0) { - result = Date.parse(`${base} ${withoutExtras}`); - } - } - - // Some browser implementations of Date aren't very flexible with the time formats. - // E.g. Safari doesn't support AM/PM or padded numbers. As a final resort, we try - // parsing some of the more common time formats ourselves. - if (!result) { - const parsed = value.toUpperCase().match(TIME_REGEX); - - if (parsed) { - let hours = parseInt(parsed[1]); - const minutes = parseInt(parsed[2]); - let seconds: number | undefined = parsed[3] == null ? undefined : parseInt(parsed[3]); - const amPm = parsed[4] as 'AM' | 'PM' | undefined; - - if (hours === 12) { - hours = amPm === 'AM' ? 0 : hours; - } else if (amPm === 'PM') { - hours += 12; - } - - if ( - inRange(hours, 0, 23) && - inRange(minutes, 0, 59) && - (seconds == null || inRange(seconds, 0, 59)) - ) { - return this.setTime(today, hours, minutes, seconds || 0); - } - } - } - - if (result) { - const date = new Date(result); - - // Firefox allows overflows in the time string, e.g. 25:00 gets parsed as the next day. - // Other browsers return invalid date objects in such cases so try to normalize it. - if (this.sameDate(today, date)) { - return date; + result = this._parseTimeString(withoutExtras); } } - return this.invalid(); + return result || this.invalid(); } override addSeconds(date: Date, amount: number): Date { @@ -397,6 +350,44 @@ export class NativeDateAdapter extends DateAdapter { d.setUTCHours(date.getHours(), date.getMinutes(), date.getSeconds(), date.getMilliseconds()); return dtf.format(d); } + + /** + * Attempts to parse a time string into a date object. Returns null if it cannot be parsed. + * @param value Time string to parse. + */ + private _parseTimeString(value: string): Date | null { + // Note: we can technically rely on the browser for the time parsing by generating + // an ISO string and appending the string to the end of it. We don't do it, because + // browsers aren't consistent in what they support. Some examples: + // - Safari doesn't support AM/PM. + // - Firefox produces a valid date object if the time string has overflows (e.g. 12:75) while + // other browsers produce an invalid date. + // - Safari doesn't allow padded numbers. + const parsed = value.toUpperCase().match(TIME_REGEX); + + if (parsed) { + let hours = parseInt(parsed[1]); + const minutes = parseInt(parsed[2]); + let seconds: number | undefined = parsed[3] == null ? undefined : parseInt(parsed[3]); + const amPm = parsed[4] as 'AM' | 'PM' | undefined; + + if (hours === 12) { + hours = amPm === 'AM' ? 0 : hours; + } else if (amPm === 'PM') { + hours += 12; + } + + if ( + inRange(hours, 0, 23) && + inRange(minutes, 0, 59) && + (seconds == null || inRange(seconds, 0, 59)) + ) { + return this.setTime(this.today(), hours, minutes, seconds || 0); + } + } + + return null; + } } /** Checks whether a number is within a certain range. */