Skip to content
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

Stop calendar blinking on DateRangePickerInput focus switch (fixes #1523) #1553

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@GfxKai
Copy link

GfxKai commented Feb 19, 2019

Using isStartDateFocused and isEndDateFocused to conditionally render children causes the calendar to disappear and reappear whenever the focus switches from the start date input to end date input.

This pr adopts same approach as dd9d6b2 by skipping the focus check and just rendering the children.

Stop calendar blinking on DateRangePickerInput focus switch (fixes #1523
)

Using `isStartDateFocused` and `isEndDateFocused` to conditionally render `children` causes a re-render whenever the focus switches from the start date input to end date input, during which the calendar disappears and reappears.

This pr adopts same approach as dd9d6b2 by skipping the focus check and just rendering the children.
@burtek

This comment has been minimized.

Copy link

burtek commented Mar 4, 2019

@GfxKai are you working on this PR?

@GfxKai

This comment has been minimized.

Copy link
Author

GfxKai commented Mar 4, 2019

@burtek it only involves 2 very minor changes, so no idea why the travis build failed the first time round but I think that blocked review / merging. I've just updated the branch and the builds have now passed, so hopefully someone can pick this up and merge it!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 4, 2019

Coverage Status

Coverage increased (+0.02%) to 84.503% when pulling 1dd632b on GfxKai:patch-1 into 32b5f3d on airbnb:master.

@burtek

This comment has been minimized.

Copy link

burtek commented Mar 4, 2019

@GfxKai great! @ljharb @majapw any chance for this PR? 🤔

@ljharb
Copy link
Member

ljharb left a comment

The change looks ok, but can we get some regression tests for it?

@burtek

This comment has been minimized.

Copy link

burtek commented Mar 5, 2019

BTW, why render children twice?

@GfxKai

This comment has been minimized.

Copy link
Author

GfxKai commented Mar 5, 2019

@burtek I'm wondering the same thing myself! 😅 good spot, will correct that now.

@ljharb happy to give that a go. just to clarify: are you looking for an enzyme test to check that the calendar stays visible when focus switches between the inputs?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 5, 2019

@GfxKai yes - you’re making conditional renderings unconditional, so there should be a test to ensure it doesn’t regress (the commit you referenced was fixed due to a test catching it)

@@ -269,7 +267,7 @@ function DateRangePickerInput({
regular={regular}
/>

{isEndDateFocused && children}

This comment has been minimized.

@ljharb

ljharb Mar 5, 2019

Member

This changes the rendering order - are you sure this doesn’t have any impact?

This comment has been minimized.

@GfxKai

GfxKai Mar 5, 2019

Author

Not 100% but the original conditional rendering logic changes the rendering order depending on which input is focused and the component seems fully functional in either case. I've done some manual testing and it appears to work perfectly with the prop in either position.

It might actually make more sense to move {children} down to be the last child of the div, since the other elements are all related to the input box and are positioned in a row together, whilst the calendar picker is absolute-positioned below.

This comment has been minimized.

@ljharb

ljharb Mar 5, 2019

Member

presumably there was some reason that the two childrens were not in the same place in the beginning - i'm loath to change that without understanding why.

This comment has been minimized.

@GfxKai

GfxKai Mar 5, 2019

Author

@monokrome any chance you could shed some light on the reasoning behind 06407e1 ?

This comment has been minimized.

@GfxKai

GfxKai Mar 5, 2019

Author

After a bit more playing around, I think I've worked out what's going on. If {children} is rendered before the second input, pressing tab moves the focus to DayPicker to make picking the start date possible with the keyboard. If the prop is moved after the end date input, pressing tab when the start date input is focused moves focus straight to the end date input.

Could we emulate this focusing behaviour using onTab event handlers rather than relying on the DayPicker's position in the DOM? I imagine this would require logic to:

  1. Focus the DayPicker on tab when the start date input is focused
  2. Focus the end date input on tab when start date input is focused and the DayPicker's next month button is focused
  3. Focus the DayPicker on tab when the end date input is focused

This comment has been minimized.

@backwardok

backwardok Mar 6, 2019

Member

We actually want to move away from the "use the down arrow key" functionality since it's not something that's obvious to a keyboard user. Additionally, for any screen reader user who is not using a keyboard (such as on a mobile device), that direction doesn't make any sense.

Similarly, we can't rely on using tab listeners to move focus to the calendar because not all screen reader users will be navigating using the tab key, even if they're using a keyboard to navigate.

One of the main reasons why it doesn't work well to have the calendar be after all elements (the way it was before) is that if you were to tab through everything to get to the calendar, you'd actually be setting the end date instead of the start date since the end date was the last field that had focus. Setting the end date in the calendar first then puts focus back on the start date field. If you then tried to tab forward again to the calendar, you'd be setting the end date again and never be able to set the start date from the calendar.

This comment has been minimized.

@GfxKai

GfxKai Mar 6, 2019

Author

Perhaps the desired behaviour could be achieved by dynamically defining the tabIndex property for the relevant input and button components, such that the order of keyboard navigation is defined depending on isStartDateFocused and isEndDateFocused. This way, the navigation order can be controlled without having to unmount and remount the DayPicker

This comment has been minimized.

@backwardok

backwardok Mar 6, 2019

Member

That was something that @monokrome had considered before, but the biggest problem with using tabindex to handle navigation is it doesn't quite work the way you may expect it to. Setting tabindex to a positive value essentially moves it earlier in natural tab flow. For an entire document, if all the tab-able elements were in an order list based on the DOM order, a positive tabindex value would move from their original spot in that list to the beginning of the list. What that means is that if you have focus on something that is in the natural order, moving focus forward won't get you to the positive tabindex elements. You'd have to manually set the tabindex for every element on the page, which would be a maintenance nightmare (and fairly fragile).

I created this codepen to demonstrate that: https://codepen.io/backwardok/pen/eQbYEa

This comment has been minimized.

@GfxKai

GfxKai Mar 7, 2019

Author

Understood. Perhaps the DayPicker could be unconditionally rendered just after the first day picker? This isn't a perfect solution, as it requires a shift-tab to go back and set the end date, but this might potentially make more sense than the DayPicker moving itself to be in a different tab position than before.

If this is not a viable alternative, I would have to argue that the re-rendering of the calendar is a much bigger issue than having to use the slightly unintuitive down arrow for keyboard navigation. The unmount and remount of the calendar is not just undesirable because of the flashing but, depending on the chosen start date, this behaviour can change which months are visible and move the location of the initially selected date to a different panel. This is extremely confusing and could easily result in users picking the wrong set of dates without realising.

Whilst I understand the need to strive for better accessibility, I think any solution that results in this re-rendering issue must be ruled out, as it has a substantially negative impact on the UX for all users 😬

This comment has been minimized.

@backwardok

backwardok Mar 14, 2019

Member

@GfxKai interesting solution! That might be a decent workaround for now, and solves some of the other concerns. For screen reader users, we could include some extra information for the checkout field that says something like, "navigate backward to interact with the calendar". It's not a perfect solution, since I don't know if it'll be as obvious to a sighted keyboard user that they could access the calendar by navigating backward and not forward, but I think that's a decent option, and one that still addresses the original problem.

Julian Regan and others added some commits Mar 6, 2019

@GfxKai

This comment has been minimized.

Copy link
Author

GfxKai commented Mar 15, 2019

@backwardok I've had a go of implementing the discussed solution. How does that look now?

@backwardok
Copy link
Member

backwardok left a comment

I like this approach! Overall looks good from an a11y perspective, with one comment

@@ -235,6 +236,8 @@ function DateRangePickerInput({
regular={regular}
/>

{children}

This comment has been minimized.

@backwardok

backwardok Mar 15, 2019

Member

should this be checking if either one is focused? seems like this would result in the calendar always showing up, even when the fields are blurred.

This comment has been minimized.

@GfxKai

GfxKai Mar 15, 2019

Author

{children} here is the DayPicker, passed through all the way from DateRangePicker, where it is conditionally rendered based on maybeRenderDayPickerWithPortal(). This function renders null here if isOpened(), which checks whether one of the two inputs are focused, returns false. Hence, the focus check is done in the (grand?)parent and we can just render unconditionally here and not worry 🎉

@noratarano
Copy link

noratarano left a comment

This looks reasonable. Thank you for your contributions @GfxKai !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.