New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: disable tab for outside days (#1567) #1576
Conversation
"If a day is both outside and selected, the equivalent "inside" day must also be present on one of the displayed calendars" I just realized this is not true, whoops. Perhaps I could check if the day is within more than one displayed month 🤔 |
I updated the PR to check if a day is both outside and displayed, which I think makes sense. |
Hey @DanielJKelly thanks for your PR, I've been investigating #1567 too and this one seems a good solution.
I want to give a second look: I'm not 100% sure of this (yet) :) |
focusContext.focusTarget && isSameDay(focusContext.focusTarget, day) | ||
focusContext.focusTarget && | ||
isSameDay(focusContext.focusTarget, day) && | ||
!isOutsideAndDisplayed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to check if the date is displayed or not. I'd just check the activeModifiers.outside
.
!isOutsideAndDisplayed | |
!activeModifiers.outside |
@DanielJKelly Thanks for your investigation! I could review your PR and apply some changes:
I think we should fix this in getInitialFocusTarget, but this function is not aware of the outside days yet. What do you think? Am I missing something if we don't check "displayed days"? |
@gpbl Thanks for the feedback and making the changes. I think you are right that checking "displayed days" is unnecessary. I only added that extra check because I wasn't completely sure if there was some use case I was missing for having an outside day be focusable. |
Context
Please see #1567 for more context about the issue this PR is meant to fix.
Analysis
The issue is that
const isFocusTarget = Boolean(focusContext.focusTarget && isSameDay(focusContext.focusTarget, day))
will return true for multiple days if there is a day that is both "selected" and "outside".If a day is both outside and selected, the equivalent "inside" day must also be present on one of the displayed calendars, so this day will receive tabIndex=0 and there is no need for the outside version of the day to have tabIndex=0.
Solution
Per the above analysis, my solution is to not consider a day with both the selected and outside active modifiers as a focus target, meaning the tabIndex will be set to -1.