[EuiSuperDatePicker] Support onFocus (#4924)

[UzairNoman]

Summary

I have made a PR for onFocus support on EuiSuperDatePicker (Issue: #4924). In addition, I have written a test to verify if the focus is infact triggering once when the input is focused.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Added documentation

• Props have proper autodocs and playground toggles
• Added or updated jest and cypress tests
• A changelog entry exists and is marked appropriately

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[cla-checker-service]

💚 CLA has been signed

[miukimiu]

Hi @UzairNoman,

Thank you for opening this PR to fix the issue #4924.

Could you please sign the Contributor Agreement so we can review your PR?

[UzairNoman]

Hi @miukimiu, sorry about that. I had signed it earlier as well. I have re-signed it again. Let me know if you guys received it. Thanks :)

[miukimiu]

Thanks, @UzairNoman. Now it's signed. ✅

An engineer will do the review as soon as possible.

[cee-chen]

Setting an onClick/onKeyUp on EuiFormControlLayout isn't the approach I would go with. I would strongly prefer that we instead pass the onFocus callback directly to each underlying EuiDatePickerRange (which now accepts onFocus as of #6136)

[UzairNoman]

I tried to follow this approach from ChoicePicker. But I agree its not the right approach. I have tried to introduce onFocus with buttonProps on EuiDatePopoverButton but it was invoking the function twice. I then introduce it directly on the <button> tag on date_popover_button.tsx to look at the issue more closely (you can check the latest commit update) but to no avail.

I suspect that this issue is linked to opening of Panel and therefore peeked into _popover_panel.tsx code:

return (
    <EuiPopoverPanelContext.Provider value={panelContext}>
      <EuiPanel
        className={classes}
        css={panelCSS}
        data-popover-panel
        data-popover-open={isOpen || undefined}
        {...rest}
      >
        {children}
      </EuiPanel>
    </EuiPopoverPanelContext.Provider> 

Specifically, if I comment {...rest} and {children} both, the onFocus call is then made once, but I wasn't able to find the exact cause of the issue. Any suggestion would be appreciated.

[cee-chen]

Hi Uzair! After a little bit of testing locally, the "double" focus events appear to primarily occur when the super date picker popover that each button toggles closes. This is due to the returnFocus logic used by our focus trap library & logic.

Whenever a dialogue that traps focus within itself (popovers, modals, and flyouts) closes, EUI attempts to return focus to the button that toggled the popover. This works great for keyboard users and escape key presses, but provides an extra "noisy" focus event in scenarios like this for mouse users who click out of a popover into another focusable item (e.g. clicking between the start date and end date button/inputs). You can see this behavior more clearly by changing your onFocus to log the event target, e.g. onFocus: (e) => console.log(e.target, new Date()).

To be honest, the extra onFocus events don't totally bother me and it feels like it is 'working as intended' (programmatically, the button is briefly recapturing focus on popover close). Do you feel like the extra event is prohibitively annoying to consumers?

[UzairNoman]

Hi Constance, thanks for putting your time into this. Let's say a user wants to attach an API call to this onFocus event or maybe just as simple as popping an element from some stack. How would he prevent it from happening twice then?

It seems like, then still the solution through onClick/onKeyUp (although a very bad workaround) creates an expected behavior from the user's standpoint, rather than this one.

[cee-chen]

Let's say a user wants to attach an API call to this onFocus event or maybe just as simple as popping an element from some stack. How would he prevent it from happening twice then?

I would personally suggest wrapping a requestAnimationFrame(() => { if (e.target === document.activeElement) ... } in their logic to ensure the event trigger is still the current focus/active element before continuing.

The dupe event may not be an issue for consumers depending on what they're doing (e.g. just checking that any button within EuiSuperDatePicker has focus), but I consider it a lesser evil over conflating event types.

To be honest we have other "dupe"/empty focus/blur/change events in other places in EUI - this would not be the first and in my opinion is not a bug / can be worked around. It is simply accurately representing the events natively occurring under the hood, and I tend to err on the side of exposing/relying on native events over smoothing them over if there isn't a specific UX goal in mind.

It seems like, then still the solution through onClick/onKeyUp (although a very bad workaround) creates an expected behavior from the user's standpoint, rather than this one.

It depends on what the consumer is trying to accomplish - "expected behavior" is entirely dependent on the goal in mind. Honestly, the concept of 'focusing into EuiSuperDatePicker' is an inherently questionable one, because there's 4-5 different items within what we consider "the EuiSuperDatePicker component" that can all individually take focus. This PR considers the "date/time inputs" (not really inputs, they're actually buttons toggling popovers) to be what matters for focus, but another consumer might want to know about focus on every single button. Without an actual UX story/case in mind, it's difficult to write code in a vacuum for the unknown.

I do feel very strongly about not conflating onClick/onKeyUp with onFocus. They're simply not the same type of event whatsoever and will end up creating event type headaches down the road vs sticking to a consistent API/event type.

If we want to really make a super robust focus behavior, my suggestion would be setting a custom onFocus and onBlur handler on the parent EuiFlexGroup that checks the current document.activeElement and stores an isFocused flag in state, and call props.onFocus if that state changes from false to true. Doing so will let you more granularly examine whether focus has moved between child buttons within the EuiSuperDatePicker and not fire props.onFocus if so.

Again, that's a lot more manual JS & logic than simply letting consumers deal with duplicate onFocus events however, so without a use-case in mind I would lean towards the simpler option. But if you're interested in trying it out, feel free!

[UzairNoman]

Alright, make sense, I updated the code with buttonProps logic, and also removed the previous workaround. Please review whenever you have time :)

[cee-chen]

jenkins test this

[kibanamachine]

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

[UzairNoman]

jenkins test this

We have to re-run it again, made the corrections in the test file, suggested snippet in the test file had minor stuff missing.

[cee-chen]

This button should also be triggering the focus handler, so we should simulate focus on it as well

Suggested change

	component
	.find('button[data-test-subj="superDatePickerShowDatesButton"]')
	.simulate('click');
	component
	.find('button[data-test-subj="superDatePickerShowDatesButton"]')
	.simulate('click')
	.simulate('focus');
	expect(focusMock).toBeCalledTimes(1);

[cee-chen]

Changes look good! One minor comment remaining. Once staging/CI finishes, I'll quickly check the props table and check off the last QA item.

[UzairNoman]

Changes look good! One minor comment remaining. Once staging/CI finishes, I'll quickly check the props table and check off the last QA item.

Hi Constance, I have updated the test cases, hopefully it should look fine now.

[cee-chen]

jenkins test this

[kibanamachine]

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

[UzairNoman]

jenkins test this

@constancecchen , sorry can you re-run the tests again, my bad :/

[cee-chen]

jenkin test this

[cee-chen]

No worries, apologies for the CI slowness/slow turnaround - we're currently babysitting a Kibana upgrade and that's taking up most of our time. Props table looks good to me! I'll approve and merge once CI passes!

[cee-chen]

Thank you for your time on this - we really appreciate your community contribution!

[kibanamachine]

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