microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.6k stars 2.75k forks source link

[Bug]: TimePicker has inadequate error handling #33012

Open EA12 opened 1 month ago

EA12 commented 1 month ago

Package

react

Package version

17.0.1

React version

17.0.1

Environment

System:
    OS: Windows 11 10.0.22631
    CPU: (12) x64 12th Gen Intel(R) Core(TM) i7-1255U
    Memory: 18.91 GB / 31.68 GB
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    @fluentui/react: ^8.106.4 => 8.121.2
    @fluentui/react-hooks: ^8.3.10 => 8.8.14
    @types/react: 17.0.45 => 17.0.45
    @types/react-dom: 17.0.17 => 17.0.17
    react: 17.0.1 => 17.0.1
    react-dom: 17.0.1 => 17.0.1

Current Behavior

Hi,

the TimePicker logs an error to the console, when the given value of internalDateAnchor is not a valid date. Error is: e.getTime is not a function

This happens if you get your data from SharePoint and the value of internalDateAnchor is a iso string representation of a valid date.

There is no related error handling in function getDateAnchor of file \node_modules\@fluentui\react\lib\components\TimePicker**TimePicker.js**

With these changes, the error will be prevented:

var getDateAnchor = function (internalDateAnchor, startEnd, increments, timeRange) { if (typeof internalDateAnchor === 'string') { internalDateAnchor = new Date(internalDateAnchor); } if (!(internalDateAnchor instanceof Date) || isNaN(internalDateAnchor.getTime())) { console.error("TimePicker::Invalid date in getDateAnchor(): ", internalDateAnchor); internalDateAnchor = new Date(); } var clampedDateAnchor = new Date(internalDateAnchor.getTime()); if (timeRange) { var clampedTimeRange = clampTimeRange(timeRange); var timeRangeHours = startEnd === 'start' ? clampedTimeRange.start : clampedTimeRange.end; if (clampedDateAnchor.getHours() !== timeRangeHours) { clampedDateAnchor.setHours(timeRangeHours); } } else if (startEnd === 'end') { clampedDateAnchor.setDate(clampedDateAnchor.getDate() + 1); } clampedDateAnchor.setMinutes(0); clampedDateAnchor.setSeconds(0); clampedDateAnchor.setMilliseconds(0); return ceilMinuteToIncrement(clampedDateAnchor, increments); };

May be you can fix this in a future release.

Thanks, Ronny

Expected Behavior

The TimePicker should handle iso values and have proper error handling

Reproduction

https://codepen.io/EA12/pen/qBeaGVp?editors=1111

Steps to reproduce

Add a DatePicker and a TimePicker and assign value, defaultValue and dateAnchor. Example:

<DatePicker disabled={props.CanEditAccidentReport === false} value={selectedAccidentDate} strings={(globalContext.language === "en") ? DayPickerStrings : DayPickerStringsGerman} onSelectDate={onAccidentDateChange} allowTextInput={true} formatDate={ (date) => { return Utility.formatDate(date); } } /> <TimePicker disabled={props.CanEditAccidentReport === false} dateAnchor={selectedAccidentDate} onChange={onAccidentTimeChange} value={currentAccidentTime ? new Date(currentAccidentTime) : new Date()} defaultValue={currentAccidentTime ? new Date(currentAccidentTime) : new Date()} />

Are you reporting an Accessibility issue?

None

Suggested severity

Medium - Has workaround

Products/sites affected

No response

Are you willing to submit a PR to fix?

no

Validations

TristanWatanabe commented 1 month ago

Can confirm repro of issue when internalDateAnchor is a string representation of a valid date, it crashes because getDateAnchor assumes that internalDateAnchor is of type Date object

tudorpopams commented 4 weeks ago

Hello folks, support for v8 components is very limited from our side, but we do accept contributions, so feel free to submit a PR if you have a solution for this problem.