EuiColorPicker uses standard portal DOM positioning, intentional popover entrance #2038

thompsongl · 2019-06-12T22:24:21Z

Summary

Uses the idea behind #1988 to address positioning inconsistency when EuiColorPicker is placed in a popover. Often, the problem can be "fixed" with position: fixed, but we ~~can~~ should do better.

The immediate change here is that the nested EuiPortal is no longer injected within the color picker component tree. Instead, it uses its shallow, late placement in the DOM to always ensure positioning (like other EuiPopover things).

The second change would resolve #1988, if accepted. It takes the concept from #1914 (comment), which asserts that popover-aided form elements should generally behave like native select elements:

Tabbing

Tab to focus on input only
Space/Enter to open popover and focus within
Space/Enter to select and close popover (back to focus just on input)

Mouse

Click on input
Focuses input and opens popover

The fundamental changes:

No auto-opening & auto-focusing popover without user intention, which makes tabbing through a form without selection is possible.
Cyclical tabbing no longer focuses the input after leaving the last swatch, which is more inline with other EUI pseudo-form elements (EuiComboBox, et al.).
An extra button (dropdown arrow; certainly open to design ideas) that (IMO) aids VO and keyboard usability.
More manual key press handling, but better z-index and positioning management math by fully embracing the EUI Platform™.

Checklist

~~- [ ] This was checked in mobile~~

This was checked in IE11
This was checked in dark mode

~~- [ ] Any props added have proper autodocs~~

Documentation examples were added
A changelog entry exists and is marked appropriately
This was checked for breaking changes and labeled appropriately
Jest tests were updated or added to match the most common scenarios
This was checked against keyboard-only and screenreader scenarios

~~- [ ] This required updates to Framer X components~~

thompsongl · 2019-06-12T22:30:40Z

src/components/color_picker/saturation.tsx

-    top: 0,
-  });
-  const [lastColor, setlastColor] = useState<HSV | {}>({});
+export const EuiSaturation: FunctionComponent<EuiSaturationProps> = forwardRef(


This diff is unfortunate. The gist is that I added forwardRef and a single ref, which changes a bunch of indentation. Nothing important to review in the file.

chandlerprall

Code changes LGTM; Tested mouse & keyboard interaction locally, including with the added container example.

The popover is not closable by keyboard when focus is still on the color picker, tabbing off of the color picker's elements allows Escape to close the popover. I'm not sure if this should be a blocker or not.

thompsongl · 2019-06-13T17:05:37Z

The popover is not closable by keyboard when focus is still on the color picker

ENTER and ESC should close the popover pretty much wherever focus is. Where're you seeing this happen?

cchaos · 2019-06-13T19:17:21Z

I honestly don't think this component needs the dropdown arrow. It just adds noise and extra tab to get out of the input. I do think it needs better VO instruction when you tab into it since all you get is:

For me, when the picker is in a popover, and I escape to close the picker, it doesn't focus back on the input like the normal one does.

But I think that's the only flaw I saw.

chandlerprall · 2019-06-13T20:58:17Z

ENTER and ESC should close the popover pretty much wherever focus is. Where're you seeing this happen?

The color picker's popover closes, but a parent popover won't close if the input box has focus

In the above state pressing Escape has no effect.

thompsongl · 2019-06-13T21:01:15Z

better VO instruction

Yep. On it.

For me, when the picker is in a popover, and I escape to close the picker, it doesn't focus back on the input like the normal one does.

The popover thing was a mistake a made in the docs. Fixed now. I did notice that the modal steals focus no matter what, though (you can see it happen with the select in the first example here: https://elastic.github.io/eui/#/layout/modal). I'm going to open an issue for it.

I honestly don't think this component needs the dropdown arrow. It just adds noise and extra tab to get out of the input

I could see this (as @snide mentioned at one point) being more like EuiSelect, where the arrow is just a visual indication and does not have button qualities. Would that be better?

chandlerprall · 2019-06-13T21:01:27Z

I honestly don't think this component needs the dropdown arrow. It just adds noise and extra tab to get out of the input.

I'll dissent, but not strongly. I navigate form fields primarily with the keyboard, and I don't think I would discover that the color picker opens without some visual indication. Maybe I would try the up & down arrows to see if it some how modifies the field value, and find instead that Down opens an interactive widget.

My 2¢, take it or leave it :)

cchaos · 2019-06-13T21:10:19Z

Yeah I'm also thinking forward with the editor style controls. Having the arrow there really takes up valuable space. So I guess there is an option to show it for normal controls and hide it for compressed?

snide · 2019-06-13T21:24:27Z

where the arrow is just a visual indication and does not have button qualities

Also think the arrow, even if it's just an indication and not a button (which I still think is fine to make just visual), is helpful to be there. Even in @cchaos' mock above... yes, it does add visual noise, but even with that gradient there it lets me know the gradient itself is clickable and will probably do something (even if I don't know what that something is). I think it's useful the same way the carets are useful on old fashioned primary, filled buttons. It just telegraphs what happens next.

thompsongl · 2019-06-13T22:09:09Z

The color picker's popover closes, but a parent popover won't close if the input box has focus

Yay @chandlerprall found a popover bug. If you have an EuiPopover in an EuiPopover, the child one stops propagation of the ESC event, even if the child EuiPopover is not open: https://codesandbox.io/embed/working-eui-xvll4

I'll open a bug for this

Bug: #2042

thompsongl · 2019-06-13T22:31:49Z

Latest commit makes the arrow a visual indicator only. See how that feels.

cchaos

It looks like I'm outnumbered 😜 I'm happy that it's not a button at least and maybe we can revisit just the "compressed" version when those get implemented.

thompsongl added 3 commits June 12, 2019 14:45

use standrd portalingl; internal focus lock; additional docs

6e15d04

ie11 and i18n fix; snapshot updates

c944076

update tests

c879c43

thompsongl requested review from cchaos, chandlerprall and snide June 12, 2019 22:24

remove test scss

61a6b43

thompsongl commented Jun 12, 2019

View reviewed changes

Merge branch 'master' into colorpicker-portaling

9826e50

chandlerprall reviewed Jun 13, 2019

View reviewed changes

allow return to input

4d10fd2

fix popover

0ab94c8

arrow as visual cue only

8d777ae

cchaos approved these changes Jun 14, 2019

View reviewed changes

CL entry

d9ead82

thompsongl merged commit 878c963 into elastic:master Jun 14, 2019

thompsongl deleted the colorpicker-portaling branch June 14, 2019 15:02

thompsongl mentioned this pull request Jun 25, 2019

Upgrade EUI to 12.1.0 elastic/kibana#39601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EuiColorPicker uses standard portal DOM positioning, intentional popover entrance #2038

EuiColorPicker uses standard portal DOM positioning, intentional popover entrance #2038

thompsongl commented Jun 12, 2019 •

edited

Loading

thompsongl Jun 12, 2019 •

edited

Loading

chandlerprall left a comment

thompsongl commented Jun 13, 2019

cchaos commented Jun 13, 2019

chandlerprall commented Jun 13, 2019

thompsongl commented Jun 13, 2019

chandlerprall commented Jun 13, 2019

cchaos commented Jun 13, 2019

snide commented Jun 13, 2019 •

edited

Loading

thompsongl commented Jun 13, 2019 •

edited

Loading

thompsongl commented Jun 13, 2019

cchaos left a comment

EuiColorPicker uses standard portal DOM positioning, intentional popover entrance #2038

EuiColorPicker uses standard portal DOM positioning, intentional popover entrance #2038

Conversation

thompsongl commented Jun 12, 2019 • edited Loading

Summary

Checklist

thompsongl Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

thompsongl commented Jun 13, 2019

cchaos commented Jun 13, 2019

chandlerprall commented Jun 13, 2019

thompsongl commented Jun 13, 2019

chandlerprall commented Jun 13, 2019

cchaos commented Jun 13, 2019

snide commented Jun 13, 2019 • edited Loading

thompsongl commented Jun 13, 2019 • edited Loading

thompsongl commented Jun 13, 2019

cchaos left a comment

Choose a reason for hiding this comment

thompsongl commented Jun 12, 2019 •

edited

Loading

thompsongl Jun 12, 2019 •

edited

Loading

snide commented Jun 13, 2019 •

edited

Loading

thompsongl commented Jun 13, 2019 •

edited

Loading