Feature/1067 focus trap initialfocus #1099

chandlerprall · 2018-08-08T18:49:27Z

Adds the initialFocus prop to EuiModal and EuiPopver
EuiModal forwards the prop through to FocusTrap
EuiPopover already does things around focusing, so the initialFocus functionality is replicated in the popover component instead of passing it to FocusTrap.
Updates a modal & popover example to use the new prop
Fix EuiSwitch to actually use its id prop

cjcenizal

Tested this in browser and it works great! Code LGTM too except for a couple a comments/questions.

cjcenizal · 2018-08-08T19:04:56Z

src/components/overlay_mask/overlay_mask.js

@@ -33,11 +33,12 @@ export class EuiOverlayMask extends Component {
      }
      this.overlayMaskNode.setAttribute(key, rest[key]);
    });
+
+    document.body.appendChild(this.overlayMaskNode);


Just curious, why did you move this into the constructor? Does this merit a comment to explain why it's better than being in componentDidMount?

Given this React DOM tree:

<a> <b> <c>

React will mount the tree into the document and then call componentDidMount in the order c -> b -> a. FocusTrap activates the focus in componentDidMount, but if the target element is not in the document an error is thrown. To ensure the target element is part of the document before FocusTrap's componentDidMount is triggered, the overlayMaskNode must be appended to the document in the constructor, as its componentDidMount happens after FocusTrap's.

Because of this, to maintain & allow natural assumptions we build about React's lifecycle, it's best practice to always best to add the custom DOM elements in the constructor.

cjcenizal · 2018-08-08T19:55:50Z

src/components/popover/popover.js

@@ -193,7 +208,9 @@ export class EuiPopover extends Component {
      }, 250);
    }

-    this.updateFocus();
+    this.updateFocus(


It looks like updateFocus doesn't accept any arguments, so is this argument necessary?

Good catch! It did something at one point during my refactoring :)

cjcenizal · 2018-08-08T20:01:36Z

src/components/modal/modal.js

@@ -32,6 +33,7 @@ export class EuiModal extends Component {
      <FocusTrap
        focusTrapOptions={{
          fallbackFocus: () => this.modal,
+          initialFocus: initialFocus,


Minor nit but I think you could just leave off the assignment:

focusTrapOptions={{ fallbackFocus: () => this.modal, initialFocus, }}

Yeah, that should be an eslint rule...

w33ble · 2018-08-08T22:09:58Z

src/components/modal/modal.js

@@ -65,4 +67,10 @@ EuiModal.propTypes = {
  className: PropTypes.string,
  children: PropTypes.node,
  onClose: PropTypes.func.isRequired,
+  /** specifies what element should initially have focus; Can be a DOM node, or a selector string (which will be passed to document.querySelector() to find the DOM node), or a function that returns a DOM node. */
+  initialFocus: PropTypes.oneOfType([


General question; do you always type check properties you don't actually use? You're not using that prop, so you don't really care what it is. You're just passing this into FocusTrap, and I assume this check matches that one, so it seems redundant. At least that's what I've been doing, but I'm open to arguments.

Couple of reasons:

enforces the interface as the boundary; yes, the prop is directly passed to FocusTrap but that's a detail I'd like to obscure

if I'm building something with EuiModal and want to know what this prop can be, I want to open up modal.js and see the answer right away without digging through any more code

our doc's find the proptypes to list based on this, specifying this prop any other way interferes with the output

w33ble · 2018-08-08T23:04:36Z

src/components/popover/popover.js

@@ -76,6 +76,13 @@ const DEFAULT_POPOVER_STYLES = {

 const GROUP_NUMERIC = /^([\d.]+)/;

+function getElementFromInitialFocus(initialFocus) {
+  const initialFocusType = typeof initialFocus;
+  if (initialFocusType === 'string') return document.querySelector(initialFocus);


document.querySelector will look on the entire page for the element to focus. You want to constrain this to just the contents of the popover, otherwise it doesn't work. Tested locally and confirmed.

Bah! This is how the upstream library works too, it just happened that the thing I tried in the modal happened to be the only item.

I still can't get things to focus correctly in the popover, but document.querySelector isn't why...

I can't figure out why, but popover focus does not work. The props look right, the node exists, it says it's calling .focus(). I tried putting a setTimeout on the focus call since that's what the FocusTrap component does, but that didn't help.

...but the node definitely does not get focused:

It should look like this when that input is focused:

Bah! This is how the upstream library works too, it just happened that the thing I tried in the modal happened to be the only item.

Yep, that's why :) I agree with your thoughts on this, I wish the upstream library didn't search the entire DOM.

Is your element display: none or visibility: hidden at the time of focus, or otherwise hidden (browser can't focus non-intractable elements)? To debug these issues when working on the PR I did the following in dev console:

const origfocus = HTMLElement.prototype.focus; HTMLElement.prototype.focus = function() { const style = window.getComputedStyle(this); console.log(this); // what element is being focused console.log(style.display, style.visibility); origfocus.call(this); };

Adding a debugger in that function also helps to inspect the rest of the DOM at time of focus, or do other tests/inspections on the target element.

Is your element display: none or visibility: hidden at the time of focus, or otherwise hidden

Nope, it's just an input in the popover content. We wrap popover, but primarily only so we can set ownFocus to true by default. We do use a render prop, so maybe that is causing some timing issue... I'll try agian without the wrapper, the issue may not be here.

w33ble · 2018-08-10T16:59:53Z

@chandlerprall here's a simple demo component I made, and the input in the popover does not get focused:

import { EuiFlexItem, EuiButton, EuiPopover } from '@elastic/eui';

class PopoverDemo extends React.PureComponent {
  state = {
    isOpen: false,
  };

  onClick = () => {
    this.setState(state => ({ isOpen: !state.isOpen }));
  };

  onClose = () => {
    this.setState(() => ({ isOpen: false }));
  };

  renderControl = () => {
    return <EuiButton onClick={this.onClick}>Show popover</EuiButton>;
  };

  render() {
    return (
      <EuiFlexItem grow={false}>
        <EuiPopover
          id="popover"
          button={this.renderControl()}
          isOpen={this.state.isOpen}
          closePopover={this.onClose}
          initialFocus="#popover-demo-input-1234"
          ownFocus
        >
          <div>Popover content</div>
          <div>
            Input: <input id="popover-demo-input-1234" />
          </div>
          <EuiButton onClick={this.onClick}>Close popover</EuiButton>
        </EuiPopover>
      </EuiFlexItem>
    );
  }
}

No dice.

Am I doing something wrong?

chandlerprall · 2018-08-10T17:39:25Z

@w33ble I copied & pasted your demo component into the EUI Popover example and it worked as expected, focusing the input box when the popover opened. Can you confirm you're using the updates from this PR?

chandlerprall · 2018-08-14T17:35:04Z

I was able to reproduce the still-problematic case of the popover not focusing correctly in Firefox.

chandlerprall · 2018-09-14T16:19:36Z

@w33ble updated to fix the popover initial focus race condition, please test again

w33ble

I can't get this to link correctly now that canvas is in the Kibana repo, so I can't really test this out. Fingers crossed you got it right!

snide · 2018-09-18T21:37:48Z

@w33ble don't forget to link within xpack as well (might have been the issue)

w33ble · 2018-09-18T22:15:23Z

@snide Yeah, I linked in both places, but the canvas plugin build step always fails because it can't resolve babel plugins... I have no idea why it doesn't work when I link EUI, but does work normally. I spent most of my morning trying to fix it to no avail.

chandlerprall · 2018-09-20T16:11:05Z

Can confirm that linking EUI breaks the canvas build, we'll need to look into that at some point. To test, I built EUI locally and manually copied src and lib directories into kibana & x-pack, and then ran canvas's yarn start. I've verified that I cannot reproduce the issue in Firefox (or Chrome) with the code from this PR. Thanks for helping debug this @w33ble !

chandlerprall added 2 commits August 8, 2018 11:53

Fix ID prop on EuiSwitch; add initialFocus prop to EuiPopover

a6e417d

Add initialFocus prop to EuiModal

2f31fb1

chandlerprall requested review from w33ble and cjcenizal August 8, 2018 18:49

cjcenizal approved these changes Aug 8, 2018

View reviewed changes

w33ble reviewed Aug 8, 2018

View reviewed changes

w33ble suggested changes Aug 8, 2018

View reviewed changes

some PR feedback

2f5c40b

chandlerprall added 4 commits September 14, 2018 09:36

Merge branch 'master' into feature/1067-focus-trap-initialfocus

810eca4

handle race condition in popover initalFocus target visibility

874b62a

changelog

80e9b1a

Merge branch 'master' into feature/1067-focus-trap-initialfocus

dcf8b16

Merge branch 'master' into feature/1067-focus-trap-initialfocus

979004f

w33ble approved these changes Sep 18, 2018

View reviewed changes

Merge branch 'master' into feature/1067-focus-trap-initialfocus

2540817

chandlerprall added 2 commits September 20, 2018 10:11

updated changelog

8ce0fd1

Merge branch 'master' into feature/1067-focus-trap-initialfocus

0326123

chandlerprall merged commit fcc2fec into elastic:master Sep 20, 2018

chandlerprall deleted the feature/1067-focus-trap-initialfocus branch September 20, 2018 16:14

snide mentioned this pull request Oct 3, 2018

Eui 4.4.1 elastic/kibana#23790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1067 focus trap initialfocus #1099

Feature/1067 focus trap initialfocus #1099

chandlerprall commented Aug 8, 2018

cjcenizal left a comment

cjcenizal Aug 8, 2018

chandlerprall Aug 9, 2018

cjcenizal Aug 8, 2018 •

edited

Loading

chandlerprall Aug 9, 2018

cjcenizal Aug 8, 2018

w33ble Aug 8, 2018

w33ble Aug 8, 2018

chandlerprall Aug 9, 2018

w33ble Aug 8, 2018 •

edited

Loading

w33ble Aug 8, 2018 •

edited

Loading

w33ble Aug 9, 2018 •

edited

Loading

chandlerprall Aug 9, 2018 •

edited

Loading

w33ble Aug 10, 2018

w33ble commented Aug 10, 2018

chandlerprall commented Aug 10, 2018

chandlerprall commented Aug 14, 2018

chandlerprall commented Sep 14, 2018

w33ble left a comment

snide commented Sep 18, 2018

w33ble commented Sep 18, 2018

chandlerprall commented Sep 20, 2018

Feature/1067 focus trap initialfocus #1099

Feature/1067 focus trap initialfocus #1099

Conversation

chandlerprall commented Aug 8, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Aug 8, 2018 • edited Loading

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

w33ble Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

w33ble Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

w33ble Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

chandlerprall Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Aug 10, 2018

chandlerprall commented Aug 10, 2018

chandlerprall commented Aug 14, 2018

chandlerprall commented Sep 14, 2018

w33ble left a comment

Choose a reason for hiding this comment

snide commented Sep 18, 2018

w33ble commented Sep 18, 2018

chandlerprall commented Sep 20, 2018

cjcenizal Aug 8, 2018 •

edited

Loading

w33ble Aug 8, 2018 •

edited

Loading

w33ble Aug 8, 2018 •

edited

Loading

w33ble Aug 9, 2018 •

edited

Loading

chandlerprall Aug 9, 2018 •

edited

Loading