Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/1067 focus trap initialfocus #1099

Merged
1 change: 1 addition & 0 deletions src-docs/src/views/modal/modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class Modal extends Component {
<EuiModal
onClose={this.closeModal}
style={{ width: '800px' }}
initialFocus="[name=popswitch]"
>
<EuiModalHeader>
<EuiModalHeaderTitle >
Expand Down
5 changes: 3 additions & 2 deletions src-docs/src/views/popover/trap_focus.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,24 @@ export default class extends Component {
button={button}
isOpen={this.state.isPopoverOpen}
closePopover={this.closePopover.bind(this)}
initialFocus="[id=asdf2]"
>
<EuiFormRow
label="Generate a public snapshot?"
id="asdf"
>
<EuiSwitch
name="switch"
id="asdf"
label="Snapshot data"
/>
</EuiFormRow>

<EuiFormRow
label="Include the following in the embed"
id="asdf2"
>
<EuiSwitch
name="switch"
id="asdf2"
label="Current time range"
/>
</EuiFormRow>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ exports[`EuiSwitch is rendered 1`] = `
aria-label="aria-label"
class="euiSwitch__input"
data-test-subj="test subject string"
id="test"
type="checkbox"
/>
<span
Expand Down
2 changes: 1 addition & 1 deletion src/components/form/switch/switch.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class EuiSwitch extends Component {
super(props);

this.state = {
id: props.id || makeId(),
switchId: props.id || makeId(),
};
}

Expand Down
8 changes: 8 additions & 0 deletions src/components/modal/modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export class EuiModal extends Component {
const {
className,
children,
initialFocus,
onClose, // eslint-disable-line no-unused-vars
...rest
} = this.props;
Expand All @@ -32,6 +33,7 @@ export class EuiModal extends Component {
<FocusTrap
focusTrapOptions={{
fallbackFocus: () => this.modal,
initialFocus: initialFocus,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that should be an eslint rule...

}}
>
{
Expand Down Expand Up @@ -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([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of reasons:

  1. enforces the interface as the boundary; yes, the prop is directly passed to FocusTrap but that's a detail I'd like to obscure
  2. 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
  3. our doc's find the proptypes to list based on this, specifying this prop any other way interferes with the output

PropTypes.instanceOf(HTMLElement),
PropTypes.func,
PropTypes.string,
]),
};
3 changes: 2 additions & 1 deletion src/components/overlay_mask/overlay_mask.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ export class EuiOverlayMask extends Component {
}
this.overlayMaskNode.setAttribute(key, rest[key]);
});

document.body.appendChild(this.overlayMaskNode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

componentDidMount() {
document.body.classList.add('euiBody-hasOverlayMask');
document.body.appendChild(this.overlayMaskNode);
}

componentWillUnmount() {
Expand Down
32 changes: 28 additions & 4 deletions src/components/popover/popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@w33ble w33ble Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@w33ble w33ble Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

@w33ble w33ble Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

screenshot 2018-08-08 17 02 24

screenshot 2018-08-08 17 04 48

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

screenshot 2018-08-08 17 02 35

It should look like this when that input is focused:

screenshot 2018-08-08 17 05 28

Copy link
Contributor Author

@chandlerprall chandlerprall Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (initialFocusType === 'function') return initialFocus();
return initialFocus;
}

export class EuiPopover extends Component {
static getDerivedStateFromProps(nextProps, prevState) {
if (prevState.prevProps.isOpen && !nextProps.isOpen) {
Expand Down Expand Up @@ -139,10 +146,18 @@ export class EuiPopover extends Component {
}

// Otherwise let's focus the first tabbable item and expedite input from the user.
const tabbableItems = tabbable(this.panel);
if (tabbableItems.length) {
tabbableItems[0].focus();
let focusTarget;

if (this.props.initialFocus != null) {
focusTarget = getElementFromInitialFocus(this.props.initialFocus);
} else {
const tabbableItems = tabbable(this.panel);
if (tabbableItems.length) {
focusTarget = tabbableItems[0];
}
}

if (focusTarget != null) focusTarget.focus();
});
}

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

this.updateFocus();
this.updateFocus(
Copy link
Contributor

@cjcenizal cjcenizal Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

!prevProps.isOpen && this.props.isOpen ? this.props.initialFocus : null
);
}

componentWillUnmount() {
Expand Down Expand Up @@ -307,6 +324,7 @@ export class EuiPopover extends Component {
panelClassName,
panelPaddingSize,
popoverRef,
initialFocus, // eslint-disable-line no-unused-vars
...rest
} = this.props;

Expand Down Expand Up @@ -436,6 +454,12 @@ EuiPopover.propTypes = {
]),
/** When `true`, the popover's position is re-calculated when the user scrolls, this supports having fixed-position popover anchors. */
repositionOnScroll: PropTypes.bool,
/** 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([
PropTypes.instanceOf(HTMLElement),
PropTypes.func,
PropTypes.string,
]),
};

EuiPopover.defaultProps = {
Expand Down