[EuiPopover] Default to ownFocus

[miukimiu]

Summary

Closes #4222.

In this PR I'm changing the default of the EuiPopover ownFocus prop from false to true.

Components without the ownFocus prop

All of the following files didn't have a ownFocus prop. So by default the ownFocus was false. So by replacing the default with true all of these components changed to ownFocus=true. I’m assuming that this change is ok and a good improvement for all the EuiPopover’s inside the following files:

• src/components/basic_table/collapsed_item_actions.tsx
• src/components/markdown_editor/markdown_editor_footer.tsx
• src/components/table/mobile/table_sort_mobile.tsx
• src/components/table/table_pagination/table_pagination.tsx
• src/components/tour/tour_step.tsx Changed to ownFocus={false}. As mentioned by @cchaos, we want the user to be able to interact with other parts of the UI while this popover is open

Components managing their ownFocus

For the following files I didn’t change anything because they have their own way to manage the ownFocus:

• src/components/color_picker/color_picker.tsx | ownFocus={popoverShouldOwnFocus}
• src/components/color_picker/color_stops/color_stop_thumb.tsx | ownFocus={isPopoverOpen}
• src/components/datagrid/data_grid_header_cell.tsx | ownFocus={isFocused}
• src/components/form/super_select/super_select.tsx | ownFocus={false}
• src/components/popover/input_popover.tsx | ownFocus={false}
• src/components/selectable/selectable_templates/selectable_template_sitewide.tsx | ownFocus={!!popoverTrigger}

Components with the ownFocus prop

I also searched for components which had the ownFocus prop (true). Now that the ownFocus by default is true I removed the prop:

• src/components/datagrid/column_selector.tsx
• src/components/datagrid/column_sorting.tsx
• src/components/datagrid/data_grid_cell_popover.tsx
• src/components/datagrid/style_selector.tsx
• src/components/date_picker/super_date_picker/quick_select_popover/quick_select_popover.tsx
• src/components/header/header_links/header_links.tsx
• src/components/search_bar/filters/field_value_selection_filter.tsx
• src/components/table/mobile/table_sort_mobile.tsx
• src/components/date_picker/super_date_picker/date_popover/date_popover_button.tsx

Tests

All the tests in src/components/popover/popover.test.tsx didn’t have the ownFocus prop. It means the ownFocus was set to false. I left the components without the prop. So it means the ownFocus now is true.

• There was one test failing that I had to set the ownFocus prop to false | closePopover is called when ESC key is hit and the popover is open
• I also add the ownFocus={false} to the test ownFocus defaults to false Changed the test title to ownFocus defaults to true and removed the prop. As mentioned by @chandlerprall, this keeps with the original intent of the test to verify the default value

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
• [ ] Checked Code Sandbox works for the any docs examples
• 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]

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

[kibanamachine]

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

[kibanamachine]

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

[kibanamachine]

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

[cchaos]

I think the only one we don't want ownFocus to be true is on the Tour component. Because they're instructional, we want the user to be able to interact with other parts of the UI while this popover is open.

There was one test failing that I had to set the ownFocus prop to false | closePopover is called when ESC key is hit and the popover is open

Is this a limitation of the test or is this a bug in ownFocus=true?

[miukimiu]

@cchaos, I changed the ownFocus to false on the Tour component.

This is the error of the test once the ownFocus is changed to true:

But @chandlerprall is taking a look.

[kibanamachine]

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

[chandlerprall]

There was one test failing that I had to set the ownFocus prop to false | closePopover is called when ESC key is hit and the popover is open

Is this a limitation of the test or is this a bug in ownFocus=true?

Dug in, when ownFocus is set to true, we rely on the underlying focus trap library's handling of Escape. When ownFocus is false, we add our own keyDown listener to the wrapping the button & the [portalled] popover content. The focus trap library's listener is added to document, which enzyme doesn't mount components into which prevents the event from bubbling to the listener.

Using ownFocus={false} for this test is fine.

[cchaos]

I think this all makes sense. I couldn't think of another EUI component that would want this to default to false besides EuiTour. 👍

Just had one comment about the tests.

[kibanamachine]

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

[cchaos]

LGTM!

[chandlerprall]

[Michail is out until Wednesday, should be able to get his review then]

[myasonik]

Love this PR. Good to merge as is for sure. I hate opening this issue across all of Kibana.

The one thing I'd add, though could be fine as its own ticket as well, is to change the color_picker default to true instead of false as well.

[chandlerprall]

Resolved the conflict & am merging this to include it in the release

[kibanamachine]

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