[Controls] [PresentationUtil] QoL improvements to control creation form #162067

Heenawter · 2023-07-17T15:38:47Z

Closes #162697

Summary

This PR adds a few tiny UI improvements to the control creation elements, including...

Data view picker:

Made the Data view form title respond to focus as expected

Before

After
Switched to use EuiInputPopover rather than EuiPopover
Removed the redundant popover title

Before

After

Field picker:

Made the Field form row title respond to focus as expected for all of the inner form elements

Before

After
Switched the FieldTypeFilter to use EuiInputPopover rather than EuiPopover
Removed the redundant title from the FieldTypeFilter popover

Before

After
Made changes described in (Accessibility) [EuiSelectable] Unable to make selections when second interactable element in focus eui#6627 (comment) so that, when the field type filter is closed (either via Esc or through the natural tab order), the focus returns to the search field

Before

After
If provided, the initial selected field is now brought to the top of the list

Before

After

Controls display settings:

Surrounded the Minimum width row with a div so that it can receive the id passed down from the EuiFormRow and respond to focus as expected

Before

After

Checklist

Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Heenawter · 2023-07-17T22:02:50Z

In my attempt at implementing the fix from elastic/eui#6627 (comment), I found a bug where the FieldTypeFilter popover's focus trap was interfering with my ability to manually set the focus back to the search input without a ~300ms delay - to resolve this. I worked with @cee-chen to discover a fix for this by allowing the popover to have control over its own returnFocus on close.

Therefore, this is blocked until elastic/eui#6955 is merged and available in Kibana 👍

Edit: Blocked on #162209

Heenawter · 2023-07-27T21:36:27Z

Unblocked now that #162209 is merged.

…rm-props_2023-07-14

Heenawter · 2023-07-31T15:04:48Z

src/plugins/controls/public/control_group/editor/control_editor.tsx

@@ -259,7 +259,7 @@ export const ControlEditor = ({
              <EuiFormRow
                label={ControlGroupStrings.manageControl.displaySettings.getWidthInputTitle()}
              >
-                <>
+                <div>


Switched this to a div so that it can receive the id passed down from the EuiFormRow component - not sure why, but the React fragment ignored it 🤷

Heenawter · 2023-07-31T15:06:02Z

src/plugins/presentation_util/public/components/field_picker/field_picker.tsx

@@ -50,7 +54,7 @@ export const FieldPicker = ({
          .filter((f) => typesFilter.length === 0 || typesFilter.includes(f.type as string))
          .filter((f) => (filterPredicate ? filterPredicate(f) : true)),
        ['name']
-      ),
+      ).sort((f) => (f.name === initialSelection.current ? -1 : 1)),


This brings the selected field, if provided, to the top of the list on load.

Heenawter · 2023-07-31T15:08:48Z

src/plugins/presentation_util/public/components/field_picker/field_type_filter.tsx

+        focusTrapProps={{
+          returnFocus: false, // we will be manually returning the focus to the search
+          onDeactivation: setFocusToSearch,
+        }}


This is the important change in this file - it ensures that, as described in elastic/eui#6627 (comment), the focus returns to the search field when the field type filter is closed (either via Esc or through the natural tab order).

elasticmachine · 2023-08-01T14:36:53Z

Pinging @elastic/kibana-presentation (Team:Presentation)

…wter/kibana into fix-controls-form-props_2023-07-14

ThomThomson

Ran this locally and looked through the code. Presentation team changes LGTM!

One small a11y / design question though:

I noticed that having the focus on the search input and using the arrow keys is the only way to select a field via the keyboard. If I have focus on the filter by type button I cannot use the arrow keys.

This seems strange to me? Is it at all possible to make the arrow keys work correctly when either element is focused?

If not, I wonder if we should swap their positions so that the filter by type button comes first? This might link the search input more obviously with the field selector. @andreadelrio do you have any thoughts on this?

kibana-ci · 2023-08-02T17:33:27Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: ee6da5c

Failed CI Steps

Investigations - Security Solution Cypress Tests #1

Test Failures

[job] [logs] Investigations - Security Solution Cypress Tests #1 / Alert details expandable flyout right panel overview tab insights section should display threat intelligence section should display threat intelligence section

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`presentationUtil`	155	145	-10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`controls`	199.8KB	199.9KB	+93.0B
`presentationUtil`	129.8KB	127.7KB	-2.1KB
total			-2.0KB

History

💚 Build #146395 succeeded 039c98f
💔 Build #146327 failed f49e55e
💔 Build #146282 failed 650ba4c
💔 Build #146004 failed 52d4e26
💔 Build #145721 failed bb9c0c4
💔 Build #145717 failed 15997e1eab1f1996b5700c07a4187098c3078267

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

cc @Heenawter

Heenawter · 2023-08-02T18:32:36Z

@ThomThomson

Is it at all possible to make the arrow keys work correctly when either element is focused?

Unfortunately, to my understanding, it's not - this is what @cee-chen had to say about it (from this issue that I raised with the EUI team):

In theory, what would be a "complete" fix would be to also prevent the up/down arrow keys from changing selection if the searchbox or listbox isn't the current focus [ ... ] I'm just not really certain there's a way to implement that while also implementing a way to ignore keydowns on children that shouldn't trigger selections.

With respect to,

I wonder if we should swap their positions so that the filter by type button comes first? This might link the search input more obviously with the field selector.

I am definitely interested to see what @andreadelrio thinks :)

andreadelrio · 2023-08-02T19:18:45Z

e search input and using the arrow keys is the only way to select a field via the keyboard. If I have focus on the filter by type button I cannot

@Heenawter @ThomThomson I wonder if joining them like in Discover would solve this issue? As an added bonus, I think it would look better and we'd be re-using a existing pattern.

ThomThomson · 2023-08-02T19:21:38Z

That's a great call!

cee-chen · 2023-08-02T19:31:16Z

Visually I agree it looks nice, but I'm not sure EuiSelectable yet supports a custom append/prepend option for the searchbox. We'd need to build that functionality into EUI.

I wonder if joining them like in Discover would solve this issue?

It would not solve the keyboard UX/a11y issue that Devon mentioned above. Here is the issue: the main search input needs to trigger several important keyboard shortcuts - pressing enter/space to select and up/down arrows to navigate through selections being the major ones. That is the defining standard of a W3 combobox spec.

However, EUI cannot easily extrapolate those keyboard shortcuts to any other custom nodes being rendered. We simply have no idea what their intention is and whether they want that behavior. In this instance for example, you want the enter/space key to toggle a popover, not toggle a selection. Why should the arrow keys behave differently? And if you did want it to behave differently, how would EUI be able to pass our keyboard event behaviors to you?

Heenawter · 2023-08-02T22:07:40Z

I'm not sure EuiSelectable yet supports a custom append/prepend option for the searchbox. We'd need to build that functionality into EUI.

Looks like searchProps does indeed support a custom append/prepend:

That being said, as @cee-chen suspected, it suffers from the exact same keyboard UX/a11y issues that @ThomThomson brought up.

However, EUI cannot easily extrapolate those keyboard shortcuts to any other custom nodes being rendered. We simply have no idea what their intention is and whether they want that behavior.

That is definitely fair - even in our specific case, assuming we got the following to work:

Is it at all possible to make the arrow keys work correctly when either element is focused?

What, then, is the behaviour when hitting the enter key? When the search box has the focus, pressing the enter key selects an item - but, if the field type filter button is in focus, what should happen? Should the popover be opened, or should a field be selected? It gets.... weird 😆

With all this in mind, I feel like we're still left with two options if we stick with EuiSelectable for this:

Keep the elements in the current order - follows the common pattern that searching is given priority over filtering (this is true for both Discover and Lens with respect to their field pickers), but makes keyboard controls a bit awkward
Reorder the elements - changes the typical search > filter pattern we see elsewhere, but makes keyboard controls feel more natural

cee-chen · 2023-08-03T01:00:53Z

Looks like searchProps does indeed support a custom append/prepend:

Nice! I love it when our components are more flexible than I remember 😆

What, then, is the behaviour when hitting the enter key? When the search box has the focus, pressing the enter key selects an item - but, if the field type filter button is in focus, what should happen? Should the popover be opened, or should a field be selected? It gets.... weird 😆

Yep, this is the exact conundrum :) Personally, my 2c is that it's not an accessibility issue, and it's not a huge UX friction point and is probably fine as-is.

When on the search input, EUI includes clear screen reader instructions as to how to interact with the component (arrow keys to navigate and enter key to select). If a custom interactive/focusable component is included within the EuiSelectable, it doesn't inherit those instructions, and therefore the keyboard shortcuts aren't expected - so at the very least, the behavior isn't confusing to screen reader users. And for non-SR users, the append option is IMO great and helps a lot with visual hierarchy.

andreadelrio

Design changes LGTM. Nice improvements @Heenawter !

Heenawter self-assigned this Jul 17, 2023

Heenawter changed the title ~~[PresentationUtil] Improve UI of DataViewPicker and FieldTypeFilter~~ [Controls] [PresentationUtil] Minor UI fixes to control creation form elements Jul 17, 2023

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch 2 times, most recently from ae3d46c to 78c3a6f Compare July 17, 2023 16:40

Heenawter added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Jul 17, 2023

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch from 8fdff2d to 476a6be Compare July 17, 2023 21:57

Heenawter added 4 commits July 28, 2023 10:00

Use ...other so focus impacts owning form row

8ac6ad5

Switch to EuiInputPopover

a5f8827

Switch focus back to search on filter popover close

2e07d31

Commit before rebase

332e87d

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch from ebc5266 to 332e87d Compare July 28, 2023 17:01

Heenawter added 2 commits July 28, 2023 11:22

Remove redundant titles from popovers

8ff957e

Bring selected field to the top of the list

6a98581

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch from 8467c96 to f3af677 Compare July 28, 2023 21:07

Fix linting + failing tests

3e9f301

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch 2 times, most recently from 15997e1 to fb5508d Compare July 28, 2023 21:43

Remove unused translations + clean up

bb9c0c4

Heenawter force-pushed the fix-controls-form-props_2023-07-14 branch from fb5508d to bb9c0c4 Compare July 28, 2023 21:44

Heenawter changed the title ~~[Controls] [PresentationUtil] Minor UI fixes to control creation form elements~~ [Controls] [PresentationUtil] QoL improvements to control creation form elements Jul 28, 2023

Heenawter changed the title ~~[Controls] [PresentationUtil] QoL improvements to control creation form elements~~ [Controls] [PresentationUtil] QoL improvements to control creation form Jul 28, 2023

Heenawter added 2 commits July 31, 2023 08:09

Merge branch 'main' of github.com:elastic/kibana into fix-controls-fo…

7590228

…rm-props_2023-07-14

Undo removed test

52d4e26

Heenawter commented Jul 31, 2023

View reviewed changes

logeekal added 3 commits August 1, 2023 01:16

Merge branch 'main' into fix-controls-form-props_2023-07-14

650ba4c

Merge branch 'main' into fix-controls-form-props_2023-07-14

f49e55e

Merge branch 'main' into fix-controls-form-props_2023-07-14

039c98f

Heenawter marked this pull request as ready for review August 1, 2023 14:36

Heenawter requested review from a team as code owners August 1, 2023 14:36

Heenawter added 3 commits August 2, 2023 10:05

Fix radius on options list again

5711793

Merge branch 'fix-controls-form-props_2023-07-14' of github.com:heena…

ee6da5c

…wter/kibana into fix-controls-form-props_2023-07-14

Undo fix - done in elastic#162651 instead

cdbef0a

ThomThomson approved these changes Aug 2, 2023

View reviewed changes

Heenawter closed this Aug 2, 2023

Heenawter reopened this Aug 2, 2023

andreadelrio approved these changes Aug 4, 2023

View reviewed changes

Heenawter merged commit 3763a5a into elastic:main Aug 4, 2023

Heenawter deleted the fix-controls-form-props_2023-07-14 branch August 4, 2023 17:07

kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Controls] [PresentationUtil] QoL improvements to control creation form #162067

[Controls] [PresentationUtil] QoL improvements to control creation form #162067

Heenawter commented Jul 17, 2023 •

edited

Loading

Heenawter commented Jul 17, 2023 •

edited

Loading

Heenawter commented Jul 27, 2023

Heenawter Jul 31, 2023

Heenawter Jul 31, 2023

Heenawter Jul 31, 2023 •

edited

Loading

elasticmachine commented Aug 1, 2023

ThomThomson left a comment

kibana-ci commented Aug 2, 2023

Heenawter commented Aug 2, 2023 •

edited

Loading

andreadelrio commented Aug 2, 2023

ThomThomson commented Aug 2, 2023 •

edited

Loading

cee-chen commented Aug 2, 2023 •

edited

Loading

Heenawter commented Aug 2, 2023 •

edited

Loading

cee-chen commented Aug 3, 2023 •

edited

Loading

andreadelrio left a comment

[Controls] [PresentationUtil] QoL improvements to control creation form #162067

[Controls] [PresentationUtil] QoL improvements to control creation form #162067

Conversation

Heenawter commented Jul 17, 2023 • edited Loading

Summary

Checklist

For maintainers

Heenawter commented Jul 17, 2023 • edited Loading

Heenawter commented Jul 27, 2023

Heenawter Jul 31, 2023

Choose a reason for hiding this comment

Heenawter Jul 31, 2023

Choose a reason for hiding this comment

Heenawter Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 1, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 2, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

History

Heenawter commented Aug 2, 2023 • edited Loading

andreadelrio commented Aug 2, 2023

ThomThomson commented Aug 2, 2023 • edited Loading

cee-chen commented Aug 2, 2023 • edited Loading

Heenawter commented Aug 2, 2023 • edited Loading

cee-chen commented Aug 3, 2023 • edited Loading

andreadelrio left a comment

Choose a reason for hiding this comment

Heenawter commented Jul 17, 2023 •

edited

Loading

Heenawter commented Jul 17, 2023 •

edited

Loading

Heenawter Jul 31, 2023 •

edited

Loading

Heenawter commented Aug 2, 2023 •

edited

Loading

ThomThomson commented Aug 2, 2023 •

edited

Loading

cee-chen commented Aug 2, 2023 •

edited

Loading

Heenawter commented Aug 2, 2023 •

edited

Loading

cee-chen commented Aug 3, 2023 •

edited

Loading