[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

kertal · 2020-08-24T13:00:13Z

Summary

The keys of the sidebar's field elements were generated by the index in the array. So when adding a column to the selected columns, its successor got the key of the element just added. This caused the weird behavior in this case (next element got focus, also the field details popover was displayed if it was active before ).

To prevent a short flickering in Chrome when clicking on the sidebar element, those elements get focus onClick

Fixes #72721

kertal · 2020-08-24T13:03:01Z

src/plugins/discover/public/application/components/sidebar/discover_sidebar_directive.ts

@@ -29,6 +29,5 @@ export function createDiscoverSidebarDirective(reactDirective: any) {
    ['onRemoveField', { watchDepth: 'reference' }],
    ['selectedIndexPattern', { watchDepth: 'reference' }],
    ['setIndexPattern', { watchDepth: 'reference' }],
-    ['state', { watchDepth: 'reference' }],


PR also contains a few cleanups of unused state variable

elasticmachine · 2020-08-24T14:57:05Z

Pinging @elastic/kibana-app (Team:KibanaApp)

andreadelrio · 2020-08-24T17:09:41Z

@kertal seems like there's some weird jumps of the + icon happening? Take a look at what happens when clicking on "day_of_week" and "email"

kertal · 2020-08-25T06:45:05Z

@kertal seems like there's some weird jumps of the + icon happening? Take a look at what happens when clicking on "day_of_week" and "email"

good catch, so it's because of the ownFocus property of EuiPopover, seems the first element of the list that got focus, is getting focus for milliseconds, when clicking on another side bar item. When you close the popover after clicking on 3 sidebar items, you can see the focus return to the first sidebar element. this may be browser depended behavior, what element gets focus, when you remove an element that has focus? no sure what to do here, unless manually setting focus when accessing the react element with a mouse click?

andreadelrio · 2020-08-25T20:41:11Z

@kertal seems like there's some weird jumps of the + icon happening? Take a look at what happens when clicking on "day_of_week" and "email"

good catch, so it's because of the ownFocus property of EuiPopover, seems the first element of the list that got focus, is getting focus for milliseconds, when clicking on another side bar item. When you close the popover after clicking on 3 sidebar items, you can see the focus return to the first sidebar element. this may be browser depended behavior, what element gets focus, when you remove an element that has focus? no sure what to do here, unless manually setting focus when accessing the react element with a mouse click?

@kertal Not 100% sure I'm following, could this be a bug on the EUI side then?

kertal · 2020-08-27T14:48:11Z

@kertal Not 100% sure I'm following, could this be a bug on the EUI side then?

No, it's the way Chrome chooses, which element to focus, when you remove the Popover.

you click on an sidebar element 1
the EuiPopover 1 gets focus.
you click on the sidebar element 2
for a short moment, between showing Popover 1 and Popover 2, element 1 gets focus, and therefore it displays the add button
you click on the sidebar element 3
Chrome again decides to focus element 1, the add button is displayed, until Popover 3 is displayed
....

kertal · 2020-08-27T14:48:36Z

@elasticmachine merge upstream

…focus-behavior

src/plugins/kibana_react/public/field_button/field_button.tsx

…-24-fix-discover-sidebar-focus-behavior

- when adding field

…r' of github.com:kertal/kibana into kertal-pr-2020-08-24-fix-discover-sidebar-focus-behavior

kertal · 2020-08-31T13:09:50Z

src/plugins/discover/public/application/components/sidebar/discover_field.tsx

+            if (ev.type === 'click') {
+              ev.currentTarget.focus();
+            }


@andreadelrio this should prevent Chrome from setting focus to the last focused sidebar element in case of a click

Tested this and it seems to solve the issue properly.

andreadelrio

LGTM, thanks for making the adjustments.

stratoula

Code LGTM, I tested it mainly on Chrome but also made some tests on Safari and FF, the bug is solved 🙂

streamich · 2020-09-02T08:56:34Z

src/plugins/kibana_react/public/field_button/field_button.tsx

-      <button onClick={onClick} {...buttonProps} className={buttonClasses}>
+      <button
+        onClick={(e) => {
+          if (e.type === 'click') {


Curious, if it is ever something else than 'click'?

yes, it could also be triggered by an enter key, but in this case it should already have focus

Dosant · 2020-09-02T09:11:25Z

(about flickering button)
@andreadelrio: @kertal Not 100% sure I'm following, could this be a bug on the EUI side then?
@kertal: No, it's the way Chrome chooses, which element to focus, when you remove the Popover.

I don't think this is a native behaviour.
Looks like focus locking and returning is handled by: react-focus-lock under the EUI hood https://github.com/theKashey/react-focus-lock#unmounting-and-focus-management

With that in mind, I guess there is a way for EUI popover to expose more granular apis for handling focus locking. Not sure if it worth it in this case or if we are OK with proposed solution.

streamich

~~Could you please check if we need the change in field_button.tsx, I'm thinking maybe this issue is already fixed with the other changes, but if we do~~ (confirmed with @kertal that we indeed need this), LGTM, checked on Chome/Mac.

kertal · 2020-09-02T09:34:57Z

(about flickering button)
@andreadelrio: @kertal Not 100% sure I'm following, could this be a bug on the EUI side then?
@kertal: No, it's the way Chrome chooses, which element to focus, when you remove the Popover.

I don't think this is a native behaviour.
Looks like focus locking and returning is handled by: react-focus-lock under the EUI hood https://github.com/theKashey/react-focus-lock#unmounting-and-focus-management

With that in mind, I guess there is a way for EUI popover to expose more granular apis for handling focus locking. Not sure if it worth it in this case or if we are OK with proposed solution.

@Dosant thx for the context and input! dear @chandlerprall & @thompsongl, are you a ok with the suggested solution to manually set focus before the EuiPopover takes focus over, or is there something that can be done on EUI side? thx

kertal · 2020-09-02T14:24:43Z

@elasticmachine merge upstream

…focus-behavior

chandlerprall · 2020-09-02T14:37:21Z

dear @chandlerprall & @thompsongl, are you a ok with the suggested solution to manually set focus before the EuiPopover takes focus over, or is there something that can be done on EUI side? thx

Definitely worth looking into EUI's focus trap integration more. The demonstrated behaviour is unexpected, at worst I would expect it to flash focus back to the previously clicked button, not the first item in the list. I would create an EUI issue and link to it in your onClick->focus fix, so if we can resolve the issue in EUI it's a quick path back to removing this patch.

kertal · 2020-09-02T15:14:18Z

dear @chandlerprall & @thompsongl, are you a ok with the suggested solution to manually set focus before the EuiPopover takes focus over, or is there something that can be done on EUI side? thx

Definitely worth looking into EUI's focus trap integration more. The demonstrated behaviour is unexpected, at worst I would expect it to flash focus back to the previously clicked button, not the first item in the list. I would create an EUI issue and link to it in your onClick->focus fix, so if we can resolve the issue in EUI it's a quick path back to removing this patch.

@chandlerprall Right, so I will merge this, and create an EUI issue and link it

kibanamachine · 2020-09-02T16:55:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8fe749e

Build metrics

async chunks size

id	value	diff	baseline
discover	430.7KB	+20.0B	430.6KB

page load bundle size

id	value	diff	baseline
kibanaReact	648.2KB	+70.0B	648.1KB

History

💛 Build #71243 was flaky 4f26d30
💚 Build #70881 succeeded 12acf3a
💔 Build #70875 failed 3b118c6
💚 Build #69974 succeeded 2ef14f6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…columns (elastic#75749) * Use field.name instead of idx for key in li element * onClick adds focus to button * prevent Chrome jumping back to last focused element

…columns (#75749) (#76621) * Use field.name instead of idx for key in li element * onClick adds focus to button * prevent Chrome jumping back to last focused element

…nes/processors-forms-k-s * 'master' of github.com:elastic/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx

…oleysens/kibana into ingest-pipelines/processors-forms-k-s * 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ...

* master: (340 commits) [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943) [release notes] automatically retry on Github API 5xx errors (elastic#76447) [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392) [i18n] Integrate 7.9.1 Translations (elastic#76391) [APM] Update aggregations to support script sources (elastic#76429) [Security Solution] Refactor Network Top Countries to use Search Strategy (elastic#76244) Document security settings available on ESS (elastic#76513) [Ingest Manager] Add input revision to the config send to the agent (elastic#76327) [DOCS] Identifies cloud settings for Monitoring (elastic#76579) [DOCS] Identifies Cloud settings in Dev Tools (elastic#76583) [Ingest Manager] Better default value for fleet long polling timeout (elastic#76393) [data.indexPatterns] Fix broken rollup index pattern creation (elastic#76593) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) ...

kertal added 2 commits August 24, 2020 14:26

Convert ES body of discover doc request to use fields API

0ac5b98

Use field.name instead of idx for key in li element

2a091d3

kertal commented Aug 24, 2020

View reviewed changes

Undo accidentally commited change

2ef14f6

kertal added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Discover Discover Application v7.10.0 release_note:fix v8.0.0 labels Aug 24, 2020

kertal self-assigned this Aug 24, 2020

kertal requested review from andreadelrio and wylieconlon August 24, 2020 14:46

kertal marked this pull request as ready for review August 24, 2020 14:57

kertal requested a review from a team August 24, 2020 14:57

onClick add focus to button

3b118c6

Merge branch 'master' into kertal-pr-2020-08-24-fix-discover-sidebar-…

12acf3a

…focus-behavior

elasticmachine requested a review from a team as a code owner August 27, 2020 14:48

kertal commented Aug 27, 2020

View reviewed changes

src/plugins/kibana_react/public/field_button/field_button.tsx Show resolved Hide resolved

kertal added 3 commits August 31, 2020 14:50

Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-08…

7994764

…-24-fix-discover-sidebar-focus-behavior

Prevent Chrome jumping back to last focused element

88b6e76

- when adding field

Merge branch 'kertal-pr-2020-08-24-fix-discover-sidebar-focus-behavio…

4f26d30

…r' of github.com:kertal/kibana into kertal-pr-2020-08-24-fix-discover-sidebar-focus-behavior

kertal commented Aug 31, 2020

View reviewed changes

andreadelrio approved these changes Sep 1, 2020

View reviewed changes

kertal requested a review from stratoula September 1, 2020 12:53

stratoula approved these changes Sep 2, 2020

View reviewed changes

streamich reviewed Sep 2, 2020

View reviewed changes

streamich approved these changes Sep 2, 2020

View reviewed changes

Merge branch 'master' into kertal-pr-2020-08-24-fix-discover-sidebar-…

8fe749e

…focus-behavior

kertal merged commit 7134cd7 into elastic:master Sep 3, 2020

kertal mentioned this pull request Sep 3, 2020

[7.x] [Discover] Fix sidebar element focus behavior when adding / removing columns (#75749) #76621

Merged

kertal added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Sep 3, 2020

kertal mentioned this pull request Sep 3, 2020

[EuiPopover] Bug with ownFocus when closing the popover elastic/eui#4003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

kertal commented Aug 24, 2020 •

edited

Loading

kertal Aug 24, 2020

elasticmachine commented Aug 24, 2020

andreadelrio commented Aug 24, 2020

kertal commented Aug 25, 2020

andreadelrio commented Aug 25, 2020

kertal commented Aug 27, 2020

kertal commented Aug 27, 2020

kertal Aug 31, 2020

andreadelrio Sep 1, 2020

andreadelrio left a comment

stratoula left a comment

streamich Sep 2, 2020

kertal Sep 2, 2020

Dosant commented Sep 2, 2020 •

edited

Loading

streamich left a comment •

edited

Loading

kertal commented Sep 2, 2020 •

edited

Loading

kertal commented Sep 2, 2020

chandlerprall commented Sep 2, 2020

kertal commented Sep 2, 2020

kibanamachine commented Sep 2, 2020

[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

[Discover] Fix sidebar element focus behavior when adding / removing columns #75749

Conversation

kertal commented Aug 24, 2020 • edited Loading

Summary

kertal Aug 24, 2020

Choose a reason for hiding this comment

elasticmachine commented Aug 24, 2020

andreadelrio commented Aug 24, 2020

kertal commented Aug 25, 2020

andreadelrio commented Aug 25, 2020

kertal commented Aug 27, 2020

kertal commented Aug 27, 2020

kertal Aug 31, 2020

Choose a reason for hiding this comment

andreadelrio Sep 1, 2020

Choose a reason for hiding this comment

andreadelrio left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

streamich Sep 2, 2020

Choose a reason for hiding this comment

kertal Sep 2, 2020

Choose a reason for hiding this comment

Dosant commented Sep 2, 2020 • edited Loading

streamich left a comment • edited Loading

Choose a reason for hiding this comment

kertal commented Sep 2, 2020 • edited Loading

kertal commented Sep 2, 2020

chandlerprall commented Sep 2, 2020

kertal commented Sep 2, 2020

kibanamachine commented Sep 2, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

History

kertal commented Aug 24, 2020 •

edited

Loading

Dosant commented Sep 2, 2020 •

edited

Loading

streamich left a comment •

edited

Loading

kertal commented Sep 2, 2020 •

edited

Loading