mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.14k stars 1.29k forks source link

[DatePicker] onAccept not working when using the keyboard to enter a date #8375

Open david-ic3 opened 1 year ago

david-ic3 commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Example : https://codesandbox.io/s/strange-maria-kreczc?file=/demo.tsx:708-716 (You can check in the console when onAccept is called)

Current behavior 😯

If I use the calendar and choose a new date onAccept is called with the new value. This is perfect.

However when I use the keyboard to enter a date I can't get onAccept to be called.

Expected behavior 🤔

I'd expect onAccept to be called with an {enter} or when leaving the date picker with a new valid date.

Order ID or Support key 💳 (optional)

19458

alexfauquette commented 1 year ago

This is related to #8310

Could you detail your use case of onAccept? Most of the use cases I've in mind can be done with onChange

I tend to think that expecting users to press Enter to validate the date is not a nice UX since their is no visual difference between a validated and non-validated date.

The onBlur could make sens, but tricky if the blur event is due to the focus going to the endAdorment button, or to the calendar.

david-ic3 commented 1 year ago

The scenario is a Date Picker outside a form that is used for rendering another widget or a set of widgets (i.e tables with information). We do not want too many events changing the dates as this is resource consuming (including server).

If the date picker doesn't allow manual input, no problem, clicking in the calendar will fire a newDate event.

If the date picker allows for manual input then it's a bit more tricky , and we do not want an additional event trigger button.

Scenario:

If the user has already a date, 1 Feb 2020 and wants to change to 23 Jun 2022 the onChange event will be notified with a list of valid dates (i.e. 2 Feb 2020, 23 Feb 2020, 23 Jun 2020 and eventually 23 Jun 2022), that's not a valid option.

We have different solutions (we are implementing 2)

1) use debounce (I'm not a fan of this one) 2) listen to the keypress and focusout events :

onAccept is currently more an onAcceptInCalendar.

Merci pour le beau travail.

mleister97 commented 1 year ago

@alexfauquette I am experiencing the same issue. I would like to trigger onAccept when either of the values is entered manually (onBlur would be ideal). It seems like a bug to me, as manual input via keyboard is not detected by onAccept. In my particular case, I need to make a server request whenever one of these the values are changed (to update dashboard data). However, using the onChange event, it will trigger 3 times per field (1x day, 1x month, 1x year), which is of course not ideal for my use case.

Thank you for your attention to this matter.

mleister97 commented 1 year ago

@alexfauquette anything planned on that?

alexfauquette commented 1 year ago

Sorry for not responding. The issue with server interaction is well-defined, but there are many ways to handle that problem. And modifying the onAccept can have lots of consequences. I've no clues for now about which solution would be the best.

We have a meeting on Monday to discuss those tricky topics between maintainers, so maybe I will have a better answer

Have a nice week-end

alexfauquette commented 1 year ago

Feedback from the discussion:

It seems strange to call onAccept on blur, or as a denounced version of onChange. That specific pattern decision are product specific and then should be in the product codebase. So we propose the following solution:

If you have other idea, or would like to share some codes for the demos, feel free to add comments :)

david-ic3 commented 1 year ago

In the meantime, this is our DatePicker component overwriting the onChange function. It's a mandatory controlled version

type WP<T, K extends keyof T> = T & { [P in K]-?: T[P] };

export default function Ic3DatePicker(props: WP<WP<WP<DatePickerProps<Date>, 'onChange'>, 'value'>, 'onAccept'>
    & React.RefAttributes<HTMLDivElement>
    & { isDateValid: (date: Date) => boolean }) {

    const {isDateValid} = props;

    const ref = useRef<HTMLDivElement>(null);
    const refDate = useRef<Date | undefined | null>();  // undefined for not set

    const onPropsAccept = props.onAccept;
    const onPropsChange = props.onChange;

    const onChange = useCallback((date: Date | null, context: PickerChangeHandlerContext<any>) => {

        onPropsChange && onPropsChange(date, context);

        if (date == null || isDateValid(date)) {
            refDate.current = date;
        } else {
            refDate.current = undefined;
        }

    }, [onPropsChange, isDateValid]);

    useEffect(() => {

        const current = ref.current;
        const input = current?.querySelectorAll("input").item(0);
        const button = current?.querySelectorAll("button").item(0);

        if (current && onPropsAccept) {
            const onEnter = (event: KeyboardEvent) => {
                if (event.code === "Enter") {
                    if (refDate.current !== undefined) {
                        onPropsAccept(refDate.current)
                    }
                }
            }
            const onBlur = () => {
                if (refDate.current !== undefined) {
                    if (!button?.matches(":hover")) {
                        onPropsAccept(refDate.current)
                    }
                }
            }

            current.addEventListener("keypress", onEnter)
            input?.addEventListener("blur", onBlur)

            return () => {
                current.removeEventListener("keypress", onEnter)
                input?.removeEventListener("blur", onBlur)
            }
        }

        return undefined;
    }, [ref, refDate, onPropsAccept])

    // force desktop on Cypress
    let desktopModeMediaQuery = undefined;
    if (window['Cypress']) {
        // force running cypress on desktop
        desktopModeMediaQuery = "@media all";
    }

    // overwrite onChange to push valid dates
    return <DatePicker
        desktopModeMediaQuery={desktopModeMediaQuery}
        {...props}
        className={clsx(UserKeyboardClasses.disableDeleteAndMove, props.className)}
        ref={ref}
        onChange={onChange}
        onAccept={onPropsAccept}
    />;
}
nahumzs commented 1 year ago

Couldn't figure it out if there is any demo solving this issue :/.

So, I gave it a shot and tried to add a debounce, but there's a big catch. When you use a DateTimePicker with a debounce and keyboard input, it gets all messed up when you change the time and date. It's pretty much impossible to know how fast a user will type in a DateTime value and control when to fetch the server.

To be honest, I think the best way to handle this is to keep the onChange the way it is and add an onBlur event. Right now, I'm bending over backward trying to figure out if the date has changed from before, just so I don't accidentally fetch the server when the onBlur (input) event just occurred.

/* this is the only way to access the onblur at this moment which is sad, cause you can get the adapter date just the string, and if you have locale is equal to not have anything. */

           <DateTimePicker
                          label={t("startDate")}
                          slotProps={{
                            textField: {
                              size: "small",
                              onBlur: handleBlurStartDateChange,
                            },
                          }}

And hey, it'd be awesome if we could use useImperativeHandle to access the date picker's internal date. That would make it way more flexible because relying on onChange alone might not cover all the different situations a DateTimePicker could run into.

alexfauquette commented 1 year ago

@nahumzs The demo is not already deployed, but you can access to it in the PR preview:

https://deploy-preview-8372--material-ui-x.netlify.app/x/react-date-pickers/lifecycle/#server-interaction

useImperativeHandle to access the date picker's internal date

I don't see how an imperative handler would solve this kind of problem

It's pretty much impossible to know how fast a user will type in a DateTime value and control when to fetch the server.

Yes, but the goal is not to guess user writing speed but to reduce the number of server requests. If you set a debounce at 500ms, you are sure you will never send more than 2 requests per second.