nextui-org / nextui

🚀 Beautiful, fast and modern React UI library.
https://nextui.org
MIT License
21.94k stars 1.53k forks source link

[BUG] - Date Picker Showing Wrong Year #3785

Open buchananwill opened 1 month ago

buchananwill commented 1 month ago

NextUI Version

2.4.6

Describe the bug

image

Speaks for itself really... 1987 is in the scroller, but 1986 in the focus. Which is it going to pick? Not clear...

A few scrolls later and this even more confusing collection of conflicts appears:

image

This was my code (using react-hook-form):

  const dateOfBirth = watch('dateOfBirth');

  const setDateValue = useCallback(
    (value: CalendarDate) => {
      const isoString = value
        .toDate(getLocalTimeZone())
        .toISOString()
        .split('T')[0];
      setValue('dateOfBirth', isoString);
    },
    [setValue]
  );
          <DatePicker
            name={'dateOfBirth'}
            aria-label={'Date of Birth'}
            label={'Date of Birth'}
            value={parseDate(dateOfBirth)}
            showMonthAndYearPickers={true}
            onChange={setDateValue}
          />

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

Use the date picker in a component. Could it be related to using a controlled value?

Expected behavior

I would expect the highlighted, focused and selected values to all be in agreement.

Screenshots or Videos

No response

Operating System Version

Browser

Chrome

linear[bot] commented 1 month ago

ENG-1377 [BUG] - Date Picker Showing Wrong Year

buchananwill commented 1 month ago

I was also getting this error on the console at the time:

Blocked aria-hidden on a <div> element because the element that just received focus must not be hidden from assistive technology users. Avoid using aria-hidden on a focused element or its ancestor. Consider using the inert attribute instead, which will also prevent focus. For more details, see the aria-hidden section of the WAI-ARIA specification at https://w3c.github.io/aria/#aria-hidden. <div class=​"flex flex-col relative overflow-hidden h-auto text-foreground box-border bg-content1 outline-none data-[focus-visible=true]​:​z-10 data-[focus-visible=true]​:​outline-2 data-[focus-visible=true]​:​outline-focus data-[focus-visible=true]​:​outline-offset-2 shadow-medium rounded-large transition-transform-background motion-reduce:​transition-none mt-8 w-64" tabindex=​"-1">​flex</div>​

subhamengine commented 1 month ago

Hey, i would like to work on this issue.

wingkwong commented 1 month ago

@subhamengine go ahead

subhamengine commented 1 month ago

sure. How can I reproduce it?

buchananwill commented 1 month ago

I would advise against looking into this directly, in fact. The issue seems to go significantly deeper than NextUI. There is/are a bug(s) in the react-aria (or spectrum?) dependency library, around parsing dates and timezones. I've spent about 3-4 hours now chasing this behaviour down, and only found more bugs! E.g.: https://github.com/adobe/react-spectrum/issues/7065

buchananwill commented 1 month ago

The only concrete thing I've determined, is that the set syncing between the month/year picker and main view is off-by-one, and glitches every time it is shown/hidden.

https://github.com/user-attachments/assets/d1df4bb0-03ed-4543-9c61-ac112565113b

buchananwill commented 1 month ago

Here is a code sandbox minimum project though. The tailwind is via CDN, because Tailwind -/-> codesandbox, but the behaviour is the same:

https://codesandbox.io/p/sandbox/j864t8

buchananwill commented 1 month ago

Two hours of breakpoint stepping later, and I believe I have the solution. Correct me if I've misunderstood any of this code.

This function is doing the actual data updating, by checking which div intersects the little grey highlight bar:

const handleListScroll = useCallback(
    (e: Event, highlightEl: HTMLElement | null, list: CalendarPickerListType) => {
      if (!(e.target instanceof HTMLElement)) return;

      const map = getItemsRefMap(list === "months" ? monthsItemsRef : yearsItemsRef);

      const items = Array.from(map.values());

It loops through the items in ascending order, and stops when it finds the intersecting div...

      const item = items.find((itemEl) => {
        const rect1 = itemEl.getBoundingClientRect();
        const rect2 = highlightEl?.getBoundingClientRect();

        if (!rect2) {
          return false;
        }

...Except when it doesn't. because the previous div is able to slightly overlap with the highlight as well. At 125% zoom on my PC, the bug goes away. Only at that zoom setting though. The rest of the time it's broken.

        return areRectsIntersecting(rect1, rect2);
      });

      const itemValue = Number(item?.getAttribute("data-value"));

      if (!itemValue) return;

Having stopped one element too early, the focusedDate is set to the wrong value.

      let date = state.focusedDate.set(list === "months" ? {month: itemValue} : {year: itemValue});

      state.setFocusedDate(date);
    },
    [state, isHeaderExpanded],
  );

My proposal:

Delete both these functions:

 const handleListScroll = useCallback(
    (e: Event, highlightEl: HTMLElement | null, list: CalendarPickerListType) => {
      if (!(e.target instanceof HTMLElement)) return;

      const map = getItemsRefMap(list === "months" ? monthsItemsRef : yearsItemsRef);

      const items = Array.from(map.values());

      const item = items.find((itemEl) => {
        const rect1 = itemEl.getBoundingClientRect();
        const rect2 = highlightEl?.getBoundingClientRect();

        if (!rect2) {
          return false;
        }

        return areRectsIntersecting(rect1, rect2);
      });

      const itemValue = Number(item?.getAttribute("data-value"));

      if (!itemValue) return;

      let date = state.focusedDate.set(list === "months" ? {month: itemValue} : {year: itemValue});

      state.setFocusedDate(date);
    },
    [state, isHeaderExpanded],
  );

  useEffect(() => {
    // add scroll event listener to monthsListRef
    const monthsList = monthsListRef.current;
    const yearsList = yearsListRef.current;
    const highlightEl = highlightRef.current;

    if (!highlightEl) return;

    const debouncedHandleMonthsScroll = debounce(
      (e: Event) => handleListScroll(e, highlightEl, "months"),
      SCROLL_DEBOUNCE_TIME,
    );
    const debouncedHandleYearsScroll = debounce(
      (e: Event) => handleListScroll(e, highlightEl, "years"),
      SCROLL_DEBOUNCE_TIME,
    );

    monthsList?.addEventListener("scroll", debouncedHandleMonthsScroll);
    yearsList?.addEventListener("scroll", debouncedHandleYearsScroll);

    return () => {
      if (debouncedHandleMonthsScroll) {
        monthsList?.removeEventListener("scroll", debouncedHandleMonthsScroll);
      }
      if (debouncedHandleYearsScroll) {
        yearsList?.removeEventListener("scroll", debouncedHandleYearsScroll);
      }
    };
  }, [handleListScroll]);

Update the data directly, when we already had the exact UI element that the user interacted with, which has the relevant data bound to it

  function scrollTo(value, list, smooth = true) {

    const mapListRef = list === "months" ? monthsItemsRef : yearsItemsRef;
    const listRef = list === "months" ? monthsListRef : yearsListRef;
// Update the focusedDate directly here.
    let date2 = state.focusedDate.set(list === "months" ? { month: itemValue } : { year: itemValue });
    state.setFocusedDate(date2);
    const map = getItemsRefMap(mapListRef);
    const node = map.get(value);
    if (!node)
      return;
    scrollIntoView(node, {
      scrollMode: "always",
      behavior: smooth ? "smooth" : "auto",
      boundary: listRef.current
    });
  }
buchananwill commented 1 month ago

Thanks. Did you need any further action from me now on this issue?

On Wed, 25 Sept 2024, 12:56 աӄա, @.***> wrote:

Assigned #3785 https://github.com/nextui-org/nextui/issues/3785 to @buchananwill https://github.com/buchananwill.

— Reply to this email directly, view it on GitHub https://github.com/nextui-org/nextui/issues/3785#event-14397862067, or unsubscribe https://github.com/notifications/unsubscribe-auth/BBL2M6LWWJJTGQVMQ2IQ3LDZYKQFZAVCNFSM6AAAAABOQ5YFZSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGM4TOOBWGIYDMNY . You are receiving this because you were assigned.Message ID: @.***>

luka-on-gh commented 3 weeks ago

Hi. Any updates on this issue and when can we expect the fixes to be live?

buchananwill commented 3 weeks ago

I authored a fix and finished working on it two weeks ago. As far as I was able to assess, all the behaviours are now working correctly in that PR. I haven't had any further feedback from @wingkwong or @ryo-manba since then. They may be able to answer when it might be included in a public release.

ryo-manba commented 3 weeks ago

@buchananwill Sorry for the delayed review. I've left some comment, so please take a look.