fix(CalendarMonth): does not skip a month on arrow click

[adamviktora]

What: Closes #9700

The bug was related to incorrect setting of initialDate, if dateProp was null, new Date(null) returns a valid date: 1 st Jan 1970, but in countries with UTC minus it returns 31 Dec 1969, then if the initial day is 31 it was causing issues in changeMonth function, which is now also refactored and fixed.

Other changes made:

• changeMonth refactoring (as we are not focusing dates on month change, changeMonth was simplified to always return the first day of the month)
• change year functionality refactoring
• unfocusing date, when changing year by clicking on small arrows

[patternfly-build]

Preview: https://patternfly-react-pr-9722.surge.sh

A11y report: https://patternfly-react-pr-9722-a11y.surge.sh

[thatblindgeye]

Looks like the original bug is resolved, but one thing I'm not sure about is always focusing the 1st day of the month. I'd at least expect the existing behavior where if the currently displayed month has a selected date, then that selected date receives focus when tabbing through the component.

Seems like there are a variety of ways to handle which date is focused, though. Always focusing the 1st like this PR does, MUI's datepicker looks to focus the 1st when going forward and the last date when going backward, while Material's datepicker seems to function similar to our existing behavior of the selected/previously focused date receiving focus when changing months.

@andrew-ronaldson @tlabaj wdyt?

[thatblindgeye]

This shouldn't be necessary. Clicking the input arrows for the year will place focus on the input itself

[tlabaj]

It looks like google calendar does not focus a date as scrolling through months. Adam had pinged me about htis yesterday and the PF 5.0 version did nit focus a specific month.

[adamviktora]

@thatblindgeye

1. If we want to keep the functionality of keeping focus on the selected date, when tabbing through months / years, we can go two commits back. It will also highlight the focused date with the lightblue circle.

Screen.Recording.2023-10-11.at.10.51.42.mov

2. If we want to keep the functionality of keeping focus on the selected date, but without highlighting with the lightblue circle, we can go one commit back. (same as the Material's datepicker, which actually has a bug there, if we select 31st then tab through shorter month like February, it will then focus the 28th on the upcoming months too)

Screen.Recording.2023-10-11.at.10.55.04.mov

3. We can keep focusing the 1st day without highlighting, Google Calendar does it this way.

I personally lean towards either option 1 or option 3 (although what we have now on patternfly.org is option 2), I think it may be confusing to not show the highlighted date and then focus the previously selected day on tabbing.

About this line:
focusRef.current.blur(); // will unfocus a date when changing year via up/down arrows
I think it is necessary, as we have to click / tab into the input first to focus it.

When running the code without that line, it keeps the focus at place. Because we do setShouldFocus(false), the condition in this useEffect will be false, so we don't execute focusRef.current.focus() (which would focus the 1st day).

  useEffect(() => {
    // Calendar month should not be focused on page load
    if ((shouldFocus || isDateFocused) && focusedDateValidated && focusRef.current) {
      focusRef.current.focus();
    }
  }, [focusedDate, isDateFocused, focusedDateValidated, focusRef]);

and just clicking the arrows does not focus the TextInput, so we don't blur the previous focus. It then also leads to these unexpected bugs shown in this video, where when I press arrow-down (in October 2021 in the video), it moves the focus from the 1st day, which is saved in focusedDate, but not actually focused:

Screen.Recording.2023-10-11.at.11.13.23.mov

[thatblindgeye]

@adamviktora ah okay, I'm not noticing a difference in behavior in Chrome when that line is added vs removed, but am noticing it in Firefox. Good call on that!

As far as the other focusing behavior, I don't feel super strong either way, and it's more what I might expect personally. I think what you have in this PR works really well, and it definitely simplifies the logic which is a plus, so I'd be good with the update

[adamviktora]

@thatblindgeye Oh, I did not realize it may be a browser thing, in Chrome the built-in input works more like I would expect.

[wise-king-sullyman]

Small nit/question but not something I'd block over, great work on this!

[wise-king-sullyman]

Nit: is checking that dateProp is truthy needed here? It looks like isValidDate should return false anyways if it's undefined.

[adamviktora]

Yeah, you are right, the previous issue lied in this line: const initDate = new Date(dateProp); where we create a valid date object with initial value (1st Jan 1970 / 31st Dec 1969), even if dateProp is undefined (for some reason if I console log dateProp, it prints out false and new Date(false) returns a valid date with that initial value).

So now that we check directly the dateProp, it is not needed.

[adamviktora]

anyway, my ESLint is shouting, because the isValidDate function only accepts Date and not Date | undefined, so can I keep it as it is?

[wise-king-sullyman]

You could also just change isValidDate to have the argument optional since it checks for a truthy value internally, up to you though.

[wise-king-sullyman]

Same question for rangeStart here.

[jpuzz0]

I think this fix/change should have some unit tests backing it up. Maybe that can be a follow-up task.