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 7 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
219 changes: 91 additions & 128 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();
setTimeout( () => this.focus() );
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'd love to get rid of this timeout but for some mysterious reason, it's not working properly without it.

  • Try removing the timeout
  • Open the inserter
  • The search input is not focused.

I tried debugging and for some reason the call to input.focus() is not focusing the input (console logging the document.actiiveElement.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to debug, and I did not find a good solution to this, It looks like although the parent component was mounted the child is not right away in a state that we can focus, this discussion seems related https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example.

What complicates, even more, the things is that if we use withSafeTimeout, we create the z-index bug ( i tested it again).

Maybe we can move the setTimeout inside the focus function so inside the timeout we don't reference this, props, etc.. we reference the dom node something like:

		const focusNode = ( domNode ) => setTimeout( () => domNode.focus() );
		const domNode = this.contentNode.current;
		// Find first tabbable node within content and shift focus, falling
		// back to the popover panel itself.
		const firstTabbable = focus.tabbable.find( domNode )[ 0 ];
		focusNode( firstTabbable ? firstTabbable : domNode );

Copy link
Contributor Author

@youknowriad youknowriad May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't expect this to create any issue regarding "this" because it's during mounting but even though, I like your proposal let's move it to focus and add a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can also add a comment specifying why we are using a set timeout, and why withSafeTimeout was not possible to use.

}

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() );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: maybe we should save the id returned by requestAnimationFrame and invoke window.cancelAnimationFrame() when unMounting if we have a valid id to cancel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already the case I believe

}

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;
}

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

throttledSetOffset() {
this.rafHandle = window.requestAnimationFrame( this.setOffset );
}

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;
8 changes: 8 additions & 0 deletions components/popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
z-index: z-index( ".components-popover" );
left: 50%;

&.is-mobile {
top: 0;
left: 0;
right: 0;
bottom: 0;
}

&:not(.is-mobile) {
margin-left: 2px;

Expand Down Expand Up @@ -82,6 +89,7 @@
position: absolute;
min-width: 240px;
height: auto;
overflow-y: auto;
}

.components-popover:not(.is-mobile).is-top & {
Expand Down
Loading