-
Notifications
You must be signed in to change notification settings - Fork 842
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
Changes from 8 commits
a6e417d
2f31fb1
2f5c40b
810eca4
874b62a
80e9b1a
dcf8b16
979004f
2540817
8ce0fd1
0326123
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,11 +33,12 @@ export class EuiOverlayMask extends Component { | |
} | ||
this.overlayMaskNode.setAttribute(key, rest[key]); | ||
}); | ||
|
||
document.body.appendChild(this.overlayMaskNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this React DOM tree:
React will mount the tree into the document and then call 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() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ...but the node definitely does not get focused: It should look like this when that input is focused: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope, it's just an input in the popover content. We wrap popover, but primarily only so we can set |
||
if (initialFocusType === 'function') return initialFocus(); | ||
return initialFocus; | ||
} | ||
|
||
export class EuiPopover extends Component { | ||
static getDerivedStateFromProps(nextProps, prevState) { | ||
if (prevState.prevProps.isOpen && !nextProps.isOpen) { | ||
|
@@ -139,10 +146,25 @@ 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); | ||
// there's a race condition between the popover content becoming visible and this function call | ||
// if the element isn't visible yet (due to css styling) then it can't accept focus | ||
// so wait for another render and try again | ||
const visibility = window.getComputedStyle(focusTarget).visibility; | ||
if (visibility === 'hidden') { | ||
this.updateFocus(); | ||
} | ||
} else { | ||
const tabbableItems = tabbable(this.panel); | ||
if (tabbableItems.length) { | ||
focusTarget = tabbableItems[0]; | ||
} | ||
} | ||
|
||
if (focusTarget != null) focusTarget.focus(); | ||
}); | ||
} | ||
|
||
|
@@ -311,6 +333,7 @@ export class EuiPopover extends Component { | |
hasArrow, | ||
repositionOnScroll, // eslint-disable-line no-unused-vars | ||
zIndex, // eslint-disable-line no-unused-vars | ||
initialFocus, // eslint-disable-line no-unused-vars | ||
...rest | ||
} = this.props; | ||
|
||
|
@@ -444,6 +467,12 @@ EuiPopover.propTypes = { | |
repositionOnScroll: PropTypes.bool, | ||
/** By default, popover content inherits the z-index of the anchor component; pass zIndex to override */ | ||
zIndex: PropTypes.number, | ||
/** 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 = { | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of reasons:
FocusTrap
but that's a detail I'd like to obscureEuiModal
and want to know what this prop can be, I want to open upmodal.js
and see the answer right away without digging through any more code