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.

Sign up for GitHub

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

Popover: Refactor popover to clarify computations #6799

Merged
merged 11 commits into from
May 23, 2018
Merged

Popover: Refactor popover to clarify computations #6799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3a34c44
Popover: Refactor popover to clarify computations and support setting…
youknowriad May 17, 2018
66b3444
Fix popover z-index
youknowriad May 17, 2018
05e8db6
Adding unit tests
youknowriad May 17, 2018
63cf64c
Fix inserter popover
youknowriad May 17, 2018
a445099
Remove useless check
youknowriad May 17, 2018
5884da6
Fix e2e tests
youknowriad May 21, 2018
2e1f8d3
Fix unit tests
youknowriad May 22, 2018
8788478
All the popovers should show up above the UI
youknowriad May 22, 2018
9ad4297
Leave more space for the arrow
youknowriad May 22, 2018
a96d4cd
Set timeout moved to the focus handler
youknowriad May 23, 2018
3e1d6be
Split the popover utility to smaller functions and test the mobile case
youknowriad May 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions components/autocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,18 +535,12 @@ export class Autocomplete extends Component {
event.stopPropagation();
}

getWordRect( { isLeft, isRight } ) {
getWordRect() {
const { range } = this.state;
if ( ! range ) {
return;
}
if ( isLeft ) {
const rects = range.getClientRects();
return rects[ 0 ];
} else if ( isRight ) {
const rects = range.getClientRects();
return rects[ rects.length - 1 ];
}

return range.getBoundingClientRect();
}

Expand Down
225 changes: 94 additions & 131 deletions components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,20 @@
* External dependencies
*/
import classnames from 'classnames';
import { isEqual, noop } from 'lodash';
import { noop } from 'lodash';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Component, createRef } from '@wordpress/element';
import { focus } from '@wordpress/dom';
import { keycodes } from '@wordpress/utils';

/**
* Internal dependencies
*/
import './style.scss';
import { computePopoverPosition } from './utils';
import withFocusReturn from '../higher-order/with-focus-return';
import PopoverDetectOutside from './detect-outside';
import IconButton from '../icon-button';
Expand All @@ -31,54 +32,42 @@ const { ESCAPE } = keycodes;
* @type {String}
*/
const SLOT_NAME = 'Popover';
const isMobile = () => window.innerWidth < 782;

class Popover extends Component {
constructor() {
super( ...arguments );

this.focus = this.focus.bind( this );
this.bindNode = this.bindNode.bind( this );
this.getAnchorRect = this.getAnchorRect.bind( this );
this.setOffset = this.setOffset.bind( this );
this.throttledSetOffset = this.throttledSetOffset.bind( this );
this.computePopoverPosition = this.computePopoverPosition.bind( this );
this.throttledComputePopoverPosition = this.throttledComputePopoverPosition.bind( this );
this.maybeClose = this.maybeClose.bind( this );

this.nodes = {};
this.contentNode = createRef();
this.anchorNode = createRef();

this.state = {
forcedYAxis: null,
forcedXAxis: null,
popoverLeft: null,
popoverTop: null,
yAxis: 'top',
xAxis: 'center',
contentHeight: null,
contentWidth: null,
isMobile: false,
popoverSize: null,
};
}

componentDidMount() {
this.setOffset();
this.setForcedPositions();
const popoverSize = this.updatePopoverSize();
this.computePopoverPosition( popoverSize );
this.toggleWindowEvents( true );
this.focus();
}

componentWillReceiveProps( nextProps ) {
if ( this.props.position !== nextProps.position ) {
this.setState( {
forcedYAxis: null,
forcedXAxis: null,
} );
}
}

componentDidUpdate( prevProps, prevState ) {
const { position } = this.props;
const { position: prevPosition } = prevProps;

if ( position !== prevPosition ) {
this.setOffset();
this.setForcedPositions();
} else if ( ! isEqual( this.state, prevState ) ) {
// Need to update offset if forced positioning applied
this.setOffset();
componentDidUpdate( prevProps ) {
if ( prevProps.position !== this.props.position ) {
this.computePopoverPosition();
}
}

Expand All @@ -90,8 +79,15 @@ class Popover extends Component {
const handler = isListening ? 'addEventListener' : 'removeEventListener';

window.cancelAnimationFrame( this.rafHandle );
window[ handler ]( 'resize', this.throttledSetOffset );
window[ handler ]( 'scroll', this.throttledSetOffset, true );
window[ handler ]( 'resize', this.throttledComputePopoverPosition );
window[ handler ]( 'scroll', this.throttledComputePopoverPosition, true );
}

throttledComputePopoverPosition( event ) {
if ( event.type === 'scroll' && this.contentNode.current.contains( event.target ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

It took me more than a minute to grasp why this condition is here. It seems obvious in retrospect, but a comment would have been nice.

return;
}
this.rafHandle = window.requestAnimationFrame( () => this.computePopoverPosition() );
}

focus() {
Expand All @@ -100,27 +96,22 @@ class Popover extends Component {
return;
}

const { content } = this.nodes;
if ( ! content ) {
if ( ! this.contentNode.current ) {
Copy link
Member

Choose a reason for hiding this comment

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

Related to earlier comment, we shouldn't need to check this here. If the component causes Popover#focus to be called during its componentDidMount, we should be able to assume that the ref.current is assigned.

return;
}

// Without the setTimeout, the dom node is not being focused
Copy link
Member

Choose a reason for hiding this comment

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

This is treating the symptom, not the cause, and the comment reads as such.

The real cause is a combination of:

  • When initially mounted, the popover doesn't have a Slot context in which to render. When this becomes available, it re-renders the content into the slot (an unmount and remount operation).
  • When initially mounted, the container has a style of visibility: hidden, so focusing would not select the container.

Copy link
Member

Choose a reason for hiding this comment

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

See also: #8218

// Related https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example
const focusNode = ( domNode ) => setTimeout( () => domNode.focus() );
Copy link
Member

Choose a reason for hiding this comment

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

Where are we canceling this timeout if the component is unmounted? Why aren't we using withSafeTimeout ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did use withSafeTimeout originally but It breaks the design in a super weird way. (see 66b3444) The usage of the higher order component even if it's not doing anything to the DOM was creating some weird z-index issues, especially visible on mobile.

Copy link
Member

Choose a reason for hiding this comment

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

What about:

Where are we canceling this timeout if the component is unmounted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already done in master


// Find first tabbable node within content and shift focus, falling
// back to the popover panel itself.
const firstTabbable = focus.tabbable.find( content )[ 0 ];
if ( firstTabbable ) {
firstTabbable.focus();
} else {
content.focus();
}
}

throttledSetOffset() {
this.rafHandle = window.requestAnimationFrame( this.setOffset );
const firstTabbable = focus.tabbable.find( this.contentNode.current )[ 0 ];
focusNode( firstTabbable ? firstTabbable : this.contentNode.current );
}

getAnchorRect() {
const { anchor } = this.nodes;
const anchor = this.anchorNode.current;
if ( ! anchor || ! anchor.parentNode ) {
return;
}
Expand All @@ -141,85 +132,42 @@ class Popover extends Component {
};
}

setOffset() {
const { getAnchorRect = this.getAnchorRect, expandOnMobile = false } = this.props;
const { popover } = this.nodes;

if ( isMobile() && expandOnMobile ) {
popover.style.left = 0;
popover.style.top = 0;
popover.style.right = 0;
popover.style.bottom = 0;
if ( ! this.state.isMobile ) {
this.setState( {
isMobile: true,
} );
}
return;
}

if ( this.state.isMobile ) {
this.setState( {
isMobile: false,
} );
}

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
const isRight = 'right' === xAxis;

const rect = getAnchorRect( { isTop, isLeft, isRight } );
if ( ! rect ) {
return;
updatePopoverSize() {
const rect = this.contentNode.current.getBoundingClientRect();
if (
! this.state.popoverSize ||
rect.width !== this.state.popoverSize.width ||
rect.height !== this.state.popoverSize.height
) {
const popoverSize = {
height: rect.height,
width: rect.width,
};
this.setState( { popoverSize } );
return popoverSize;
}

popover.style.bottom = 'auto';
popover.style.right = 'auto';

// Set popover at parent node center
popover.style.left = Math.round( rect.left + ( rect.width / 2 ) ) + 'px';

// Set at top or bottom of parent node based on popover position
popover.style.top = rect[ yAxis ] + 'px';
return this.state.popoverSize;
}

setForcedPositions() {
const anchor = this.getAnchorRect();
const rect = this.nodes.content.getBoundingClientRect();

// Check exceeding top or bottom of viewport and switch direction if the space is begger
if ( rect.top < 0 || rect.bottom > window.innerHeight ) {
const overflowBottom = window.innerHeight - ( anchor.bottom + rect.height );
const overflowTop = anchor.top - rect.height;
const direction = overflowTop < overflowBottom ? 'bottom' : 'top';
if ( direction !== this.state.forcedYAxis ) {
this.setState( { forcedYAxis: direction } );
}
}
computePopoverPosition( popoverSize ) {
const { getAnchorRect = this.getAnchorRect, position = 'top', expandOnMobile } = this.props;
const newPopoverPosition = computePopoverPosition(
getAnchorRect(), popoverSize || this.state.popoverSize, position, expandOnMobile
);

// Check exceeding left or right of viewport and switch direction if the space is begger
if ( rect.left < 0 || rect.right > window.innerWidth ) {
const overflowLeft = anchor.left - rect.width;
const overflowRight = window.innerWidth - ( anchor.right + rect.width );
const direction = overflowLeft < overflowRight ? 'right' : 'left';
if ( direction !== this.state.forcedXAxis ) {
this.setState( { forcedXAxis: direction } );
}
if (
this.state.yAxis !== newPopoverPosition.yAxis ||
this.state.xAxis !== newPopoverPosition.xAxis ||
this.state.popoverLeft !== newPopoverPosition.popoverLeft ||
this.state.popoverTop !== newPopoverPosition.popoverTop ||
this.state.contentHeight !== newPopoverPosition.contentHeight ||
this.state.contentWidth !== newPopoverPosition.contentWidth ||
this.state.isMobile !== newPopoverPosition.isMobile
) {
this.setState( newPopoverPosition );
}
}

getPositions() {
const { position = 'top' } = this.props;
const [ yAxis, xAxis = 'center' ] = position.split( ' ' );
const { forcedYAxis, forcedXAxis } = this.state;

return [
forcedYAxis || yAxis,
forcedXAxis || xAxis,
];
}

maybeClose( event ) {
const { onKeyDown, onClose } = this.props;

Expand All @@ -235,10 +183,6 @@ class Popover extends Component {
}
}

bindNode( name ) {
return ( node ) => this.nodes[ name ] = node;
}

render() {
const {
headerTitle,
Expand All @@ -257,15 +201,24 @@ class Popover extends Component {
/* eslint-enable no-unused-vars */
...contentProps
} = this.props;
const [ yAxis, xAxis ] = this.getPositions();
const {
popoverLeft,
popoverTop,
yAxis,
xAxis,
contentHeight,
contentWidth,
popoverSize,
isMobile,
} = this.state;

const classes = classnames(
'components-popover',
className,
'is-' + yAxis,
'is-' + xAxis,
{
'is-mobile': this.state.isMobile,
'is-mobile': isMobile,
}
);

Expand All @@ -276,12 +229,16 @@ class Popover extends Component {
let content = (
<PopoverDetectOutside onClickOutside={ onClickOutside }>
<div
ref={ this.bindNode( 'popover' ) }
className={ classes }
style={ {
top: ! isMobile && popoverTop ? popoverTop + 'px' : undefined,
left: ! isMobile && popoverLeft ? popoverLeft + 'px' : undefined,
visibility: popoverSize ? undefined : 'hidden',
} }
{ ...contentProps }
onKeyDown={ this.maybeClose }
>
{ this.state.isMobile && (
{ isMobile && (
<div className="components-popover__header">
<span className="components-popover__header-title">
{ headerTitle }
Expand All @@ -290,8 +247,12 @@ class Popover extends Component {
</div>
) }
<div
ref={ this.bindNode( 'content' ) }
ref={ this.contentNode }
className="components-popover__content"
style={ {
maxHeight: ! isMobile && contentHeight ? contentHeight + 'px' : undefined,
maxWidth: ! isMobile && contentWidth ? contentWidth + 'px' : undefined,
} }
tabIndex="-1"
>
{ children }
Expand All @@ -314,17 +275,19 @@ class Popover extends Component {
content = <Fill name={ SLOT_NAME }>{ content }</Fill>;
}

return <span ref={ this.bindNode( 'anchor' ) }>
return <span ref={ this.anchorNode }>
{ content }
{ this.state.isMobile && expandOnMobile && <ScrollLock /> }
{ isMobile && expandOnMobile && <ScrollLock /> }
</span>;
}
}

Popover.contextTypes = {
const PopoverContainer = Popover;

PopoverContainer.contextTypes = {
getSlot: noop,
};

Popover.Slot = () => <Slot bubblesVirtually name={ SLOT_NAME } />;
PopoverContainer.Slot = () => <Slot bubblesVirtually name={ SLOT_NAME } />;

export default Popover;
export default PopoverContainer;
Loading