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

Fixed EuiSuperDatePicker input focus. #3154

Merged
merged 15 commits into from
Apr 7, 2020
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

**Bug Fixes**

- Fixed the `initialSelectedTab` prop of `EuiTabbedContent` to not steal focus from content. Which fixed the bug in `EuiSuperDatePicker` that required two clicks to focus input in relative tab ([#3154](https://github.com/elastic/eui/pull/3154))
- Fixed the `img` element in `EuiIcon` using custom SVGs to have an `alt` attribute with an empty string, rather than no `alt` attribute at all ([#3245](https://github.com/elastic/eui/pull/3245))
- Added overflows to EuiDataGrid toolbar dropdowns when there are many columns ([#3238](https://github.com/elastic/eui/pull/3238))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,13 @@ exports[`EuiTabbedContent behavior when uncontrolled, the selected tab should up
]
}
>
<div
onFocus={[Function]}
>
<EuiTabs>
<div>
<ForwardRef
onFocus={[Function]}
>
<div
className="euiTabs"
onFocus={[Function]}
role="tablist"
>
<EuiTab
Expand Down Expand Up @@ -134,7 +135,7 @@ exports[`EuiTabbedContent behavior when uncontrolled, the selected tab should up
</button>
</EuiTab>
</div>
</EuiTabs>
</ForwardRef>
<div
aria-labelledby="kibana"
id="42"
Expand Down
25 changes: 13 additions & 12 deletions src/components/tabs/tabbed_content/tabbed_content.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class EuiTabbedContent extends Component<

private readonly rootId = htmlIdGenerator()();

private readonly divRef = createRef<HTMLDivElement>();
private readonly tabsRef = createRef<HTMLDivElement>();

constructor(props: EuiTabbedContentProps) {
super(props);
Expand All @@ -89,19 +89,19 @@ export class EuiTabbedContent extends Component<
componentDidMount() {
// IE11 doesn't support the `relatedTarget` event property for blur events
// but does add it for focusout. React doesn't support `onFocusOut` so here we are.
if (this.divRef.current) {
if (this.tabsRef.current) {
// Current short-term solution for event listener (see https://github.com/elastic/eui/pull/2717)
this.divRef.current.addEventListener(
this.tabsRef.current.addEventListener(
'focusout' as 'blur',
this.removeFocus
);
}
}

componentWillUnmount() {
if (this.divRef.current) {
if (this.tabsRef.current) {
// Current short-term solution for event listener (see https://github.com/elastic/eui/pull/2717)
this.divRef.current.removeEventListener(
this.tabsRef.current.removeEventListener(
'focusout' as 'blur',
this.removeFocus
);
Expand All @@ -113,7 +113,7 @@ export class EuiTabbedContent extends Component<
// Must wait for setState to finish before calling `.focus()`
// as the focus call triggers a blur on the first tab
this.setState({ inFocus: true }, () => {
const targetTab: HTMLDivElement | null = this.divRef.current!.querySelector(
const targetTab: HTMLDivElement | null = this.tabsRef.current!.querySelector(
`#${this.state.selectedTabId}`
);
targetTab!.focus();
Expand Down Expand Up @@ -170,12 +170,13 @@ export class EuiTabbedContent extends Component<
const { content: selectedTabContent, id: selectedTabId } = selectedTab!;

return (
<div
ref={this.divRef}
className={className}
{...rest}
onFocus={this.initializeFocus}>
<EuiTabs expand={expand} display={display} size={size}>
<div className={className} {...rest}>
<EuiTabs
ref={this.tabsRef}
expand={expand}
display={display}
size={size}
onFocus={this.initializeFocus}>
Copy link
Contributor

Choose a reason for hiding this comment

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

@ashikmeerankutty I talked with @chandlerprall and we came to the conclusion that the divRef is still need in order to allow tabbing through all of the tabs. But that it should also be move to <EuiTabs>. By doing so you'll most likely need to modify EuiTabs to forwardRef to the containing div.

Copy link
Contributor Author

@ashikmeerankutty ashikmeerankutty Apr 4, 2020

Choose a reason for hiding this comment

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

@cchaos Thanks for the comment. Moved divRef to EuiTabs. Do I need to rename divRef to tabRef for readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would help a lot. Perhaps even tabsRef (plural) so it's more obvious that it's on the whole EuiTabs component.

{tabs.map((tab: EuiTabbedContentTab) => {
const {
id,
Expand Down
57 changes: 33 additions & 24 deletions src/components/tabs/tabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,28 +34,37 @@ export type EuiTabsProps = CommonProps &
size?: EuiTabsSizes;
};

export const EuiTabs = ({
children,
className,

display = 'default',
expand = false,
size = 'm',
...rest
}: PropsWithChildren<EuiTabsProps>) => {
const classes = classNames(
'euiTabs',
displayToClassNameMap[display],
sizeToClassNameMap[size],
export type EuiTabRef = HTMLDivElement;

export const EuiTabs = React.forwardRef<
EuiTabRef,
PropsWithChildren<EuiTabsProps>
>(
(
{
'euiTabs--expand': expand,
},
className
);

return (
<div role="tablist" className={classes} {...rest}>
{children}
</div>
);
};
children,
className,
display = 'default',
expand = false,
size = 'm',
...rest
}: PropsWithChildren<EuiTabsProps>,
ref
) => {
const classes = classNames(
'euiTabs',
displayToClassNameMap[display],
sizeToClassNameMap[size],
{
'euiTabs--expand': expand,
},
className
);

return (
<div ref={ref} role="tablist" className={classes} {...rest}>
{children}
</div>
);
}
);