Fix EuiComboBox focus trap #866

cjcenizal · 2018-05-23T02:42:15Z

Alternative approach to #845. Fixes #788 and #787. Also fixes a bug in which clicking the caret doesn't set focus to the search input.

This also seems to fix a bug in master, in which opening the list, using the down arrow to navigate to an item, and then hitting tab ends up setting focus to limbo. In this PR, you can't escape the list by tabbing. You have to hit Escape to return focus to the search input, and then you can tab away.

@nreese @chandlerprall What do you think of this approach? If you think this has promise then I need to update the CHANGELOG with the many breaking changes, update other instances in which EuiFormControlLayout is consumed, and fix and flesh out the tests.

cjcenizal · 2018-05-23T02:55:00Z

src/components/form/form_control_layout/form_control_layout.js

-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.shape({


Thoughts on this pattern? I was inspired a bit by https://www.youtube.com/watch?v=Yy7gFgETp0o&t=1079s.

I think this is a smarter pattern. One thing I think we probably should do is improve how we'll handle this kind of layout in our autodocs though. While importing the array values was never great and had similar problems, at least you could see the prop names themselves (which is probably the most important part). Right now it will be super vague and blackboxy. Most of the time when we do this kind of abstraction we actually only expose subsets of the original proplist (for example, color is not coming over from icon) so it will be harder to say "consult EuiIcon for props...etc).

Ultimately up to y'all, so I'll stay out of the decision here, but it'd be nice to have some sort of plan should we go down this route (if we are planning to adopt / migrate this pattern in more places where we nest components, which is what I'm imagining you two are thinking). Obviously this is a mostly internal component so the breaking change is less damaging than most, but if we started changing things like EuiButton with this pattern, I think we'd need to really consider the impact to benefit we're gaining.

chandlerprall · 2018-05-23T16:53:05Z

Do we know what browser/OS combinations do or don't allow tabbing out of a native select box? Chrome/Mac at least does not let you tab out of the expanded options list.

cjcenizal · 2018-05-23T17:00:08Z

On Mac:

Safari blocks you from tabbing out of the expanded options list.
Firefox will select the option you've highlighted and close the list.

This might be another case where we get to make up the rules a bit.

chandlerprall · 2018-05-23T18:00:24Z

This approach does look a lot cleaner to me than #845

nreese · 2018-05-23T18:31:52Z

This approach does look a lot cleaner to me than #845

agreed. I will close 845 in favor of this

chandlerprall

needs a changelog but otherwise LGTM

cjcenizal · 2018-05-23T20:09:41Z

src/components/combo_box/combo_box.js

  componentDidMount() {
    this._isMounted = true;
+    document.addEventListener('click', this.onDocumentFocusChange);
+    document.addEventListener('focusin', this.onDocumentFocusChange);


By leaving these event listeners active while the component is mounted, we can open the list when the user tabs backwards to give the clear button focus.

cjcenizal · 2018-05-23T20:14:59Z

@chandlerprall @nreese I updated the tests, made some minor tweaks, and updated the CHANGELOG. Could you take another look please?

chandlerprall · 2018-05-23T20:17:59Z

src/components/combo_box/combo_box.js

      }
+
+      // Otherwise tab to the next adjacent item.
+      tabbableItems[searchInputIndex + amount].focus();


test if this exceeds tabbableItems.length?

chandlerprall · 2018-05-23T20:20:27Z

src/components/combo_box/combo_box.js

+
+      // Wrap to last tabbable if tabbing backwards.
+      if (amount < 0) {
+        if (searchInputIndex === 0) {


amount may be less that -1, test if searchInputIndex + amount < 0

- Set focus on searchInput when you click the caret to open the combo box. - Once open, clicking it again is a no-op.

- Redesign EuiFormControlLayout props to use icon and clear configuration objects.

cjcenizal · 2018-05-23T20:49:44Z

@chandlerprall Thanks for the review! I addressed your comments by adding a check that the amount can only be -1 or 1 (which seems reasonable since this should only accept valid tabbing increments). This also solves issues where we tab beyond tabbableItems.length, since the other conditions in this method will wrap focus to the beginning of the tab order. Could you take another look?

chandlerprall

one last change

chandlerprall · 2018-05-23T20:53:28Z

src/components/combo_box/combo_box.js

@@ -137,26 +137,42 @@ export class EuiComboBox extends Component {
  };

  tabAway = amount => {
+    if (![-1, 1].includes(amount)) {


IE doesn't support includes. What do you think of if (Math.abs(amount) !== 1) ?

Oh interesting! We're using includes in other places in EUI. Maybe we should just add documentation stating that we expect consumers to polyfill ES2015 features, e.g. with babel-polyfill. I think that's fair, since EUI is intended for usage within Elastic and I'd be surprised if we couldn't meet that requirements.

Sounds like the right thing since the methods are already being used

nreese · 2018-05-23T21:24:38Z

This breaks the functionality of being able to close the list by clicking the caret. I think that is a nice feature to have is used in kibana functional tests.

cjcenizal · 2018-05-24T22:51:37Z

I had to refactor EuiFormControlLayout significantly because it wasn't placing the DOM elements in the correct order when the icon is on the right with the clear button. @snide @cchaos Could you take a look at these layouts and make sure they make sense from a design perspective?

snide

Padding stuff I'd consider a blocker, rest is just asking questions. Code quality is sharp as always.

snide · 2018-05-25T03:25:05Z

src/components/form/form_control_layout/_form_control_layout.scss

-    // Loading spinner needs adjustment if clear also exists
-    ~ .euiFormControlLayout__loading {
-      right: $euiFormControlPadding*3;
+    > * + * {


This is more readable to me, but you'll still need a way to figure out how to calculate padding on the input.

Looking at this again, it's only a problem in the generic examples and not the real form controls themselves. So ignore the above. But we might want to provide a comment about the issue if someone wants to use this stuff.

^^ Well, kinda. See the comment below about the number field.

Good idea, I'll add a note to the docs.

snide · 2018-05-25T03:57:18Z

src/components/form/form_control_layout/form_control_layout.js

-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([
+    PropTypes.string,
+    PropTypes.shape({


I think this is a smarter pattern. One thing I think we probably should do is improve how we'll handle this kind of layout in our autodocs though. While importing the array values was never great and had similar problems, at least you could see the prop names themselves (which is probably the most important part). Right now it will be super vague and blackboxy. Most of the time when we do this kind of abstraction we actually only expose subsets of the original proplist (for example, color is not coming over from icon) so it will be harder to say "consult EuiIcon for props...etc).

Ultimately up to y'all, so I'll stay out of the decision here, but it'd be nice to have some sort of plan should we go down this route (if we are planning to adopt / migrate this pattern in more places where we nest components, which is what I'm imagining you two are thinking). Obviously this is a mostly internal component so the breaking change is less damaging than most, but if we started changing things like EuiButton with this pattern, I think we'd need to really consider the impact to benefit we're gaining.

snide · 2018-05-25T04:28:44Z

src/components/form/field_number/field_number.js

@@ -81,7 +82,14 @@ EuiFieldNumber.propTypes = {
  max: PropTypes.number,
  step: PropTypes.number,
  value: numberOrEmptyString,
-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([


A lot of these components purposefully limited prop declarations. Is there a reason to add iconSide and onClick to the number field? I kind of like giving people less options here which is why I only exposed the props in some of the compoents. Generally, when we've given people the power to use things, they start using them. I'm just a little worried about seeing random action icons on the right side of inputs that lack context. Down arrow, clear? Makes sense. Random logstash icon with some hidden meaning and an onClick action has me worried 😄

Yep. Looks like using them will actually break the layouts because of the assumptions the previous api made.

I think if these just get reverted to the strings only you're prolly ok? Leave the mapping stuff to the control only?

Oh great catch! This was an oversight on my part.

… docs with note about input padding.

cjcenizal · 2018-05-25T12:54:32Z

@snide Thanks for your feedback! I've addressed your points. About the pattern of an icon prop accepting an object, I think we'll have to consider it on a case-by-case basis. It's good in situations where the icon needs so much customization like inside of combo_box.js, but makes much less sense for something like a button's icon, where these extra props won't do much good. I think the real benefit here is that we have a new pattern to apply to props which require this extra level of customization, which we can reach for when needed.

nreese · 2018-05-25T13:44:26Z

works great

nreese

lgtm

snide

Cool. Thanks.

chandlerprall · 2018-05-25T15:41:06Z

Thanks for pushing forward on this! LGTM

cjcenizal requested review from chandlerprall and nreese May 23, 2018 02:42

cjcenizal commented May 23, 2018

View reviewed changes

nreese mentioned this pull request May 23, 2018

Combo Box is a keyboard trap #845

Closed

chandlerprall suggested changes May 23, 2018

View reviewed changes

cjcenizal force-pushed the combo-box-focus-trap branch from 791009f to 2bc2cd5 Compare May 23, 2018 20:08

cjcenizal commented May 23, 2018

View reviewed changes

cjcenizal force-pushed the combo-box-focus-trap branch from 2bc2cd5 to fecf0be Compare May 23, 2018 20:12

chandlerprall suggested changes May 23, 2018

View reviewed changes

snide and others added 4 commits May 23, 2018 13:39

style up focus for combo buttons

7f6953e

Fix bug where clicking the combo box caret would focus on the button.

e0c81c9

- Set focus on searchInput when you click the caret to open the combo box. - Once open, clicking it again is a no-op.

Fix EuiComboBox focus trap. Make clear button keyboard-accessible.

a5e37c8

- Redesign EuiFormControlLayout props to use icon and clear configuration objects.

Protect against tab amounts that are not 1 or -1.

3f799e4

cjcenizal force-pushed the combo-box-focus-trap branch from 9de6431 to 3f799e4 Compare May 23, 2018 20:40

Remove virtualized list from tab order.

425945e

chandlerprall suggested changes May 23, 2018

View reviewed changes

chandlerprall approved these changes May 23, 2018

View reviewed changes

cjcenizal mentioned this pull request May 24, 2018

Document requirement of an ES2015 polyfill. #870

Merged

cjcenizal added 3 commits May 24, 2018 15:51

Refactor EuiFormControlLayout and add examples of it to the docs.

f5e2aca

Add extra space to bottom of docs site pages.

e34a195

Put toggle button back into tab order.

d622d46

cjcenizal force-pushed the combo-box-focus-trap branch from 9f5c26c to d622d46 Compare May 24, 2018 22:51

cjcenizal requested review from snide and cchaos May 24, 2018 22:51

cjcenizal added 2 commits May 24, 2018 16:03

Simplify EuiFormControlLayout styles.

f4fe254

Minor polish to combo box.

26065e8

cjcenizal force-pushed the combo-box-focus-trap branch from 8fecab5 to 26065e8 Compare May 24, 2018 23:10

snide suggested changes May 25, 2018

View reviewed changes

Revert change to icon prop on EuiFieldText and EuiFieldNumber. Update…

087374c

… docs with note about input padding.

nreese approved these changes May 25, 2018

View reviewed changes

snide approved these changes May 25, 2018

View reviewed changes

cjcenizal merged commit 08aeacd into elastic:master May 25, 2018

cjcenizal deleted the combo-box-focus-trap branch May 25, 2018 15:46

snide mentioned this pull request May 25, 2018

Clear combo box button not accessible in tab order #787

Closed

cjcenizal mentioned this pull request Jun 14, 2018

Fix bug where users could tab away from EuiComboBox while navigating the options list #926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EuiComboBox focus trap #866

Fix EuiComboBox focus trap #866

cjcenizal commented May 23, 2018 •

edited

Loading

cjcenizal May 23, 2018 •

edited

Loading

chandlerprall May 23, 2018

snide May 25, 2018

chandlerprall commented May 23, 2018

cjcenizal commented May 23, 2018 •

edited

Loading

chandlerprall commented May 23, 2018

nreese commented May 23, 2018

chandlerprall left a comment

cjcenizal May 23, 2018

cjcenizal commented May 23, 2018

chandlerprall May 23, 2018

chandlerprall May 23, 2018

cjcenizal commented May 23, 2018

chandlerprall left a comment

chandlerprall May 23, 2018

cjcenizal May 23, 2018

chandlerprall May 23, 2018

nreese commented May 23, 2018

cjcenizal commented May 24, 2018 •

edited

Loading

snide left a comment

snide May 25, 2018

snide May 25, 2018

snide May 25, 2018

cjcenizal May 25, 2018

snide May 25, 2018

snide May 25, 2018

snide May 25, 2018

cjcenizal May 25, 2018

cjcenizal commented May 25, 2018 •

edited

Loading

nreese commented May 25, 2018

nreese left a comment

snide left a comment

chandlerprall commented May 25, 2018

Fix EuiComboBox focus trap #866

Fix EuiComboBox focus trap #866

Conversation

cjcenizal commented May 23, 2018 • edited Loading

cjcenizal May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerprall commented May 23, 2018

cjcenizal commented May 23, 2018 • edited Loading

chandlerprall commented May 23, 2018

nreese commented May 23, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 23, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese commented May 23, 2018

cjcenizal commented May 24, 2018 • edited Loading

snide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented May 25, 2018 • edited Loading

nreese commented May 25, 2018

nreese left a comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

chandlerprall commented May 25, 2018

cjcenizal commented May 23, 2018 •

edited

Loading

cjcenizal May 23, 2018 •

edited

Loading

cjcenizal commented May 23, 2018 •

edited

Loading

cjcenizal commented May 24, 2018 •

edited

Loading

cjcenizal commented May 25, 2018 •

edited

Loading