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.

Sign up for GitHub

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

Jump to bottom

[EuiSuperDatePicker] Support onFocus (#4924) #6320

Merged
merged 8 commits into from
Nov 7, 2022
Merged

[EuiSuperDatePicker] Support onFocus (#4924) #6320

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
12c045e
[EuiSuperDatePicker] Support onFocus (#4924)
UzairNoman Oct 22, 2022
feb1649
callback invoking multiple times.
UzairNoman Oct 28, 2022
b3aad72
Merge branch 'elastic:main' into onfocus-super-date-picker
UzairNoman Oct 28, 2022
0fec4de
[EuiSuperDatePicker] Updated the onFocus call on buttonProps and pret…
UzairNoman Nov 2, 2022
52717c0
Apply suggestions from code review
UzairNoman Nov 2, 2022
ffb6e14
[EuiSuperDatePicker] Updated super_date_picker's tests and onFocus on…
UzairNoman Nov 2, 2022
71e6086
[EuiSuperDatePicker] added test case for focus on superDatePickerShow…
UzairNoman Nov 6, 2022
87d75db
[EuiSuperDatePicker] prettier error resolved (#4924)
UzairNoman Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/components/date_picker/super_date_picker/__snapshots__/super_date_picker.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ exports[`EuiSuperDatePicker is rendered 1`] = `
className="euiSuperDatePicker testClass1 testClass2"
data-test-subj="test subject string"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -336,6 +338,8 @@ exports[`EuiSuperDatePicker props accepts data-test-subj and passes to EuiFormCo
className="euiSuperDatePicker"
data-test-subj="mySuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -661,6 +665,8 @@ exports[`EuiSuperDatePicker props compressed is rendered 1`] = `
className="euiSuperDatePicker"
compressed={true}
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -1022,6 +1028,8 @@ exports[`EuiSuperDatePicker props isQuickSelectOnly is rendered 1`] = `
<EuiFormControlLayout
className="euiSuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -1281,6 +1289,8 @@ exports[`EuiSuperDatePicker props showUpdateButton can be false 1`] = `
<EuiFormControlLayout
className="euiSuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -1584,6 +1594,8 @@ exports[`EuiSuperDatePicker props showUpdateButton can be iconOnly 1`] = `
<EuiFormControlLayout
className="euiSuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -1908,6 +1920,8 @@ exports[`EuiSuperDatePicker props width can be auto 1`] = `
<EuiFormControlLayout
className="euiSuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down Expand Up @@ -2232,6 +2246,8 @@ exports[`EuiSuperDatePicker props width can be full 1`] = `
<EuiFormControlLayout
className="euiSuperDatePicker"
isDisabled={false}
onClick={[Function]}
onKeyUp={[Function]}
prepend={
<EuiQuickSelectPopover
applyTime={[Function]}
Expand Down
15 changes: 15 additions & 0 deletions src/components/date_picker/super_date_picker/super_date_picker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,21 @@ describe('EuiSuperDatePicker', () => {
);
});

test('keyup or click on FCLayout or DatePickerRange should invoke onFocus on SuperDatePicker', () => {
const focusMock = jest.fn();

const componentFocus = mount<EuiSuperDatePickerInternal>(
<EuiSuperDatePicker onTimeChange={noop} onFocus={focusMock} />
);

componentFocus.find('EuiFormControlLayout').simulate('keyUp');
componentFocus.find('EuiDatePickerRange').simulate('keyUp');
componentFocus.find('EuiFormControlLayout').simulate('click');
componentFocus.find('EuiDatePickerRange').simulate('click');

expect(focusMock).toBeCalledTimes(4);
});

describe('showUpdateButton', () => {
test('can be false', () => {
const component = shallowAndDive(
Expand Down
19 changes: 18 additions & 1 deletion src/components/date_picker/super_date_picker/super_date_picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import React, { Component, FunctionComponent } from 'react';
import React, { Component, FocusEventHandler, FunctionComponent } from 'react';
import classNames from 'classnames';
import moment, { LocaleSpecifier } from 'moment'; // eslint-disable-line import/named
import dateMath from '@elastic/datemath';
Expand Down Expand Up @@ -87,6 +87,11 @@ export type EuiSuperDatePickerProps = CommonProps & {
*/
locale?: LocaleSpecifier;

/**
* Triggered whenever the EuiSuperDatePicker is focused
UzairNoman marked this conversation as resolved.
Show resolved Hide resolved
*/
onFocus?: FocusEventHandler<HTMLDivElement>;

/**
* Callback for when the refresh interval is fired.
* EuiSuperDatePicker will only manage a refresh interval timer when onRefresh callback is supplied
Expand Down Expand Up @@ -533,6 +538,7 @@ export class EuiSuperDatePickerInternal extends Component<
isDisabled,
isPaused,
onRefreshChange,
onFocus,
recentlyUsedRanges,
refreshInterval,
showUpdateButton,
Expand All @@ -547,6 +553,15 @@ export class EuiSuperDatePickerInternal extends Component<
// Force reduction in width if showing quick select only
const width = isQuickSelectOnly ? 'auto' : _width;

const handleInputActivity = (
event:
| React.KeyboardEvent<HTMLInputElement>
| React.MouseEvent<HTMLInputElement>
| any
) => {
if (onFocus) onFocus(event);
};

const autoRefreshAppend: EuiFormControlLayoutProps['append'] = !isPaused ? (
<EuiAutoRefreshButton
className="euiFormControlLayout__append"
Expand Down Expand Up @@ -612,6 +627,8 @@ export class EuiSuperDatePickerInternal extends Component<
compressed={compressed}
isDisabled={isDisabled}
prepend={quickSelect}
onClick={handleInputActivity}
onKeyUp={handleInputActivity}
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@cee-chen cee-chen Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

append={autoRefreshAppend}
data-test-subj={dataTestSubj}
>
Expand Down
2 changes: 2 additions & 0 deletions src/components/form/form_control_layout/form_control_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ export type EuiFormControlLayoutProps = CommonProps &
* Creates an input group with element(s) coming after children.
* `string` | `ReactElement` or an array of these
*/

onClick?: (e: React.MouseEvent<HTMLInputElement>) => void;
append?: PrependAppendType;
children?: ReactNode;
icon?: EuiFormControlLayoutIconsProps['icon'];
Expand Down
2 changes: 2 additions & 0 deletions upcoming_changelogs/6320.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Added onFocus callback on EuiSuperDatePicker
UzairNoman marked this conversation as resolved.
Show resolved Hide resolved