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

[EuiDatePicker] Allow user text entry by preventing focus trap #4243

Merged
merged 26 commits into from
Dec 17, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Nov 9, 2020

Summary

Closes #3367 by preventing the internal focus trap used by EuiDatePicker from automatically setting focus on mount.

New interaction matches that of EuiColorPicker and is as follows:

  • Click the input, the calendar popover opens but focus remains in the input (disabled focus trap)
  • Tab or arrow down key clicks activate the focus trap, moving focus inside the calendar popover.
  • Custom inputs remain with the current functionality (automatic focus trap activation) because we cannot be certain that the provided component will respect the tab/arrow activation technique.

I attempted to write a test for this, but react-popper errors on missing document methods not available in jsdom.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation

- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@thompsongl thompsongl marked this pull request as ready for review November 11, 2020 00:06
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@cchaos
Copy link
Contributor

cchaos commented Nov 11, 2020

Thanks for tackling this one. I've been running through it by using both the mouse and keyboard and here's a couple things I found:

1. Keyboard interaction doesn't work on the top controls (without time select)
As soon as the user hits enter/space on the arrows or the month/year, it automatically closes the popover. You can see this behavior in the first example.
Screen Shot 2020-11-11 at 12 01 59 PM

2. Tabbing into the input should automatically open the popover
Instead, the user has to hit the down arrow to open it

3. Hitting esc while the popover is open doesn't automatically go back to focus inside of the input
I'm not sure where the focus actually lands but I can then hit tab and it goes to the next element, so it's close just not the actual input.

4. Can we do something about aligning the input text to the calendar display?
For instance if I edit the year, it doesn't reflect in the dropdown.
Screen Shot 2020-11-11 at 12 11 14 PM

@thompsongl
Copy link
Contributor Author

1. Keyboard interaction doesn't work on the top controls (without time select)

Looking into this

2. Tabbing into the input should automatically open the popover

This is unchanged from the current behavior and different from the what happens in EuiColorPicker. I think the idea is that a user would be able to tab through the form without opening (and tabbing through) the popover if desired.

3. Hitting esc while the popover is open doesn't automatically go back to focus inside of the input

This is also unchanged from the current behavior, but I agree it should focus the input.

4. Can we do something about aligning the input text to the calendar display?
For instance if I edit the year, it doesn't reflect in the dropdown.

I can't replicate this (or I misunderstand the problem)

Screen Cast 2020-11-11 at 10 55 15 AM

@cchaos
Copy link
Contributor

cchaos commented Nov 11, 2020

2. Tabbing into the input should automatically open the popover

This is unchanged from the current behavior and different from the what happens in EuiColorPicker. I think the idea is that a user would be able to tab through the form without opening (and tabbing through) the popover if desired.

I think this brings up a good candidate for discussion. Without a screen reader, there's nothing telling keyboard users to press down to open the popover. We should probably discuss and nail down this type of behavior for all inputs with dropdowns.

4. Can we do something about aligning the input text to the calendar display?
For instance if I edit the year, it doesn't reflect in the dropdown.

I can't replicate this (or I misunderstand the problem)

Try editing the input while the popover is not open, then open the popover.

@myasonik
Copy link
Contributor

myasonik commented Nov 11, 2020

  1. Tabbing into the input should automatically open the popover

This is unchanged from the current behavior and different from the what happens in EuiColorPicker. I think the idea is that a user would be able to tab through the form without opening (and tabbing through) the popover if desired.

I think this brings up a good candidate for discussion. Without a screen reader, there's nothing telling keyboard users to press down to open the popover. We should probably discuss and nail down this type of behavior for all inputs with dropdowns.

The usual solution to this is:

  • focus on control opens whatever it needs (in this case, focus on input opens the DatePicker)
  • tab moves focus into the popover
  • esc closes the popover and puts focus on the input with the popover closed
  • tab moves focus to the next tabbable element on the page

Next time a user lands on the control (e.g., input), it starts over at the top where the popover opens. This does have the downside of after pressing esc the only way to open the popover is to tab out and back into the control but that's not often a dealbreaker.

We can explore other strategies (e.g., using a modifier to open the popover, adding a button to open the popover, etc) but starting with the usual solution might be a good immediate fix even while we discuss others.

@thompsongl
Copy link
Contributor Author

Without a screen reader, there's nothing telling keyboard users to press down to open the popover. We should probably discuss and nail down this type of behavior for all inputs with dropdowns.

Also remembered that this was the reason we added the down arrow icon (like EuiSelect has) to EuiColorPicker. But I don't think everyone was in agreement that this was an ideal solution.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@thompsongl
Copy link
Contributor Author

Try editing the input while the popover is not open, then open the popover.

Still seems ok?

Screen Cast 2020-11-16 at 2 07 08 PM

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@thompsongl
Copy link
Contributor Author

Updated interaction per @myasonik details, with the addition of leaving in the down-arrow key as an option for opening the popover after using esc to close it.

See how you like it; still open to discussing other options.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@myasonik
Copy link
Contributor

I like it! 🎉

@cchaos
Copy link
Contributor

cchaos commented Nov 17, 2020

Hmm.. It certainly happened at some point.. Maybe the date was invalid so it wouldn't update the calendar.. 🤷

I still can't seem to change the year though with keyboard navigation.

@thompsongl
Copy link
Contributor Author

I still can't seem to change the year though with keyboard navigation.

Missed that the year dropdown was part of your original review. Looking.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@chandlerprall
Copy link
Contributor

chandlerprall commented Dec 3, 2020

from #4243 (comment)

It does not seem like we want to allow strict mode to be optional, but let's discuss

@cchaos what is your opinion? I think it makes sense to always allow "incomplete" dates, as long as they're formed enough that Moment can build a proper representation from them.

@thompsongl
Copy link
Contributor Author

thompsongl commented Dec 7, 2020

as long as they're formed enough that Moment can build a proper representation from them.

Kicked the tires on Moment's "forgiving mode" date parsing, and I'm now less hesitant about using it. It respects the expected format well enough (e.g., won't parse YYYY-MM-DD if expecting MM/DD/YYYY) and the component will use a fallback time value on blur if the input is somewhat parsable (e.g., 2020-12-07 results in 2020-12-07 12:00:00:000 AM). If we want to allow forgiving mode, I think we still add a prop to enforce strict mode if consumers want.

Still would like to hear what @cchaos has to say.

@cchaos
Copy link
Contributor

cchaos commented Dec 9, 2020

I think it makes sense to always allow "incomplete" dates

YES PLEASE! This will actually satisfy a lot of Kibana customers because the Super date picker is too strict. And would close this one #1715

If we want to allow forgiving mode, I think we still add a prop to enforce strict mode if consumers want.

I'm happy with that solution!!!

@thompsongl
Copy link
Contributor Author

I'll go ahead and enable forgiving mode.

the Super date picker is too strict

And take a look at how this impacts EuiSuperDatePicker

@thompsongl
Copy link
Contributor Author

And take a look at how this impacts EuiSuperDatePicker

Updated EuiDatePicker, but EuiSuperDatePicker does not use the same date utils so is unaffected by the change. If we want to change EuiSuperDatePicker also, I'd rather do so in a separate PR.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@cchaos
Copy link
Contributor

cchaos commented Dec 14, 2020

If we want to change EuiSuperDatePicker also, I'd rather do so in a separate PR.

I'm ok with that, but I do think it would be a great improvement if we could get it done sooner than later.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. Lots of good improvements.

Only regression I can find is enter no longer closes the popover, if the input box has focus.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM!!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦑 LGTM! Could not break the implementation

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4243/

@thompsongl thompsongl merged commit 5d792a0 into elastic:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDatePicker] Can no longer change text in input
5 participants