fix: prevent focus from moving beyond toDate and fromDate

[kimamula]

Prevent focus from moving beyond toDate and fromDate to fix #1320

[gpbl]

Thanks @kimamula I'd rather rollback #1449 than adding a new prop. This would however cause #1320 to reappear again.

[kimamula]

What about always enabling the behavior I implemented behind disallowFocusOnDisabledDates?

[gpbl]

What about always enabling the behavior I implemented behind disallowFocusOnDisabledDates?

That behavior is native in the browser already when we rollback #1449. I'd say is a better solution!

[kimamula]

The behavior I implemented here prevents the issue described in #1320 from happening.

[kimamula]

@gpbl reverted #1451 and removed disallowFocusOnDisabledDates and disallowFocusOnDisabledNavigation props

[kimamula]

@gpbl FYI, @react-aria/datepicker behaves similarly to what I implemented here when minValue and/or maxValue is set.

https://react-spectrum.adobe.com/react-aria/useDatePicker.html#minimum-and-maximum-values

[kimamula]

@gpbl is it possible to estimate when you will be able to take a look at this? Thanks!

[gpbl]

The behavior I implemented here prevents the issue described in #1320 from happening.

Thanks @kimamula, I will check it out.

Still we need to fix #1320 in a PR ad-hoc, after reverting #1451. The fix should focus only on the fix and have updated tests.

If you want to help:

• revert Use aria-disabled instead of disabled #1451 in a different PR
• open a PR with the changes that would fix Focus navigation breaks with disabled days  #1320.
• this should include one or more tests that confirm the issue is fixed

If you have urgency with this issue, consider to downgrade to v8.0.4 (if the recent fixes aren't bugging you). Also sponsoring the project would help to push forward its development. Thanks!

[kimamula]

@gpbl thanks! Reverted #1451 in #1476 and updated this PR to focus on fixing #1320.

[kimamula]

@gpbl could you kindly review this PR? Downgrading to v8.0.4 doesn't work for us since #1320 is also not acceptable. Thanks!

[gpbl]

I could finally try out this PR, but there's an issue: when meeting a disabled day, the user needs to press the arrow button twice to skip the disabled day:

What was the issue and how does this PR fix it? Some context would help for a quicker review!

[gpbl]

I think we'd better close #1483 for a easier development on our local machine. Also, it seems the tests don't cover the issue. The test should confirm that the disabled days are skipped when moving with the keyboard navigation.

Thanks for your help!

[kimamula]

Thank you for your review!!

I believe this PR fixes the issue shown in the CodeSandbox example you provided in #1320. I didn't know about disabled prop and didn't address the issue you pointed out (I updated the title of this PR to avoid confusion).

It seems difficult and will take a lot of time to determine how to move the focus when encountering a disabled. It would be great if the issue could be made an item to be addressed in a separate PR. What do you think?

@react-aria/datepicker renders disabled dates without actually "disabling" them. This way, it does not have this issue as well as the issue I described in #1449.

[kimamula]

I found that the issue described in your comment is not specific to disabled but also happens with hidden, for which the solution adopted in @react-aria/datepicker doesn't work.

I still hope this issue could be addressed in a separate PR, but here are my thoughts on how to solve the issue:

• focusStartOfWeek: move to the first focusable (not disabled or hidden) day in the week
• focusEndOfWeek: move to the last focusable day in the week
• focusDayBefore, focusWeekBefore, focusMonthBefore, focusYearBefore:
  1. Move to the previous day, week, month, or year if it is selectable.
  2. If it is not, move to the nearest day to 1.
  3. If 2 is the currently focused day, move to the previous focusable day.
  4. If 3 does not exist, stay at the currently focused day.
• focusDayAfter, focusWeekAfter, focusMonthAfter, focusYearAfter:
  1. Move to the next day, week, month, or year if it is selectable.
  2. If it is not, move to the nearest day to 1.
  3. If 2 is the currently focused day, move to the next focusable day.
  4. If 3 does not exist, stay at the currently focused day.

// pseudo code (focusStartOfWeek and focusEndOfWeek are omitted)
const moveFocus = (addFn: typeof addDays, after: boolean) => {
    if (!focusedDay) return;
    let newFocusedDay = addFn(focusedDay, after ? 1 : -1);
    if (!isFocusable(newFocusedDay)) {
        newFocusedDay = findNearestFocusableDayTo(newFocusedDay);
        if (isSameDay(focusedDay, newFocusedDay) {
            newFocusedDay = after ? findNextFocusableDay(focusedDay) : findPreviousFocusableDay(focusedDay);
        }
    }
    if (!newFocusedDay) return;
    navigation.goToDate(newFocusedDay, focusedDay);
    focus(newFocusedDay);
};

const focusDayBefore = () => moveFocus(addDays, false);
const focusDayAfter = () => moveFocus(addDays, true);
const focusWeekBefore = () => moveFocus(addWeeks, false);
const focusWeekAfter = () => moveFocus(addWeeks, true);
const focusMonthBefore = () => moveFocus(addMonths, false);
const focusMonthAfter = () => moveFocus(addMonths, true);
const focusYearBefore = () => moveFocus(addYears, false);
const focusYearAfter = () => moveFocus(addYears, true);

[gpbl]

@kimamula I see thanks for the feedback! I'll send some updates for this PR, which is a good start! Thanks again for your work here.

[McSam27]

Any ETA on this branch being merged in?

[kimamula]

@gpbl is there anything I can do to get this PR merged?

[gpbl]

@gpbl is there anything I can do to get this PR merged?

Thanks @kimamula! There are few bugs surfacing listed here https://github.com/gpbl/react-day-picker/issues, it would be nice some help there too.

Now that #1483 is closed it should be easier to debug and review this PR.

@McSam27 please don't ask for ETA thanks, I'm working on this issue in my free-time 🙏

[gpbl]

Looks great @kimamula ! Thanks for your patience – I want to update in a second PR some code and ask for a review 🙏

[gpbl]

I like this change - I'd better move it in an external function tho... I will update the code when merged.