Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions editor/components/rich-text/format-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { prependHTTP } from '@wordpress/url';
* Internal dependencies
*/
import './style.scss';
import PositionedAtSelection from './positioned-at-selection';
import UrlInput from '../../url-input';
import { filterURLForDisplay } from '../../../utils/url';

Expand Down Expand Up @@ -175,7 +176,7 @@ class FormatToolbar extends Component {
}

render() {
const { formats, focusPosition, enabledControls = DEFAULT_CONTROLS, customControls = [], selectedNodeId } = this.props;
const { formats, enabledControls = DEFAULT_CONTROLS, customControls = [], selectedNodeId } = this.props;
const { linkValue, settingsVisible, opensInNewWindow } = this.state;
const isAddingLink = formats.link && formats.link.isAdding;

Expand Down Expand Up @@ -216,7 +217,7 @@ class FormatToolbar extends Component {

{ ( isAddingLink || formats.link ) && (
<Fill name="RichText.Siblings">
<div className="editor-format-toolbar__link-container" style={ { ...focusPosition } }>
<PositionedAtSelection className="editor-format-toolbar__link-container">
<Popover
position="bottom center"
focusOnMount={ isAddingLink ? 'firstElement' : false }
Expand Down Expand Up @@ -275,7 +276,7 @@ class FormatToolbar extends Component {
/* eslint-enable jsx-a11y/no-static-element-interactions */
) }
</Popover>
</div>
</PositionedAtSelection>
</Fill>
) }
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { getOffsetParent, getRectangleFromRange } from '@wordpress/dom';

/**
* Returns a style object for applying as `position: absolute` for an element
* relative to the bottom-center of the current selection. Includes `top` and
* `left` style properties.
*
* @return {Object} Style object.
*/
function getCurrentCaretPositionStyle() {
const selection = window.getSelection();

// Unlikely, but in the case there is no selection, return empty styles so
// as to avoid a thrown error by `Selection#getRangeAt` on invalid index.
if ( selection.rangeCount === 0 ) {
return {};
}

// Get position relative viewport.
const rect = getRectangleFromRange( selection.getRangeAt( 0 ) );
let top = rect.top + rect.height;
let left = rect.left + ( rect.width / 2 );

// Offset by positioned parent, if one exists.
const offsetParent = getOffsetParent( selection.anchorNode );
if ( offsetParent ) {
const parentRect = offsetParent.getBoundingClientRect();
top -= parentRect.top;
left -= parentRect.left;
}

return { top, left };
}

/**
* Component which renders itself positioned under the current caret selection.
* The position is calculated at the time of the component being mounted, so it
* should only be mounted after the desired selection has been made.
*
* @type {WPComponent}
*/
class PositionedAtSelection extends Component {
constructor() {
super( ...arguments );

this.state = {
style: getCurrentCaretPositionStyle(),
};
}

render() {
const { className, children } = this.props;
const { style } = this.state;

return (
<div className={ className } style={ style }>
{ children }
</div>
);
}
}

export default PositionedAtSelection;
45 changes: 11 additions & 34 deletions editor/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,27 +430,6 @@ export class RichText extends Component {
this.context.onCreateUndoLevel();
}

/**
* Calculates the relative position where the link toolbar should be.
*
* Based on the selection of the text inside this element a position is
* calculated where the toolbar should be. This can be used downstream to
* absolutely position the toolbar.
*
* @param {DOMRect} position Caret range rectangle.
*
* @return {{top: number, left: number}} The desired position of the toolbar.
*/
getFocusPosition( position ) {
// The container is relatively positioned.
const containerPosition = this.containerRef.current.getBoundingClientRect();

return {
top: position.top - containerPosition.top + position.height,
left: position.left - containerPosition.left + ( position.width / 2 ),
};
}

/**
* Handles a horizontal navigation key down event to handle the case where
* TinyMCE attempts to preventDefault when on the outside edge of an inline
Expand Down Expand Up @@ -752,20 +731,19 @@ export class RichText extends Component {
return accFormats;
}, {} );

let rect;
const selectedAnchor = find( parents, ( node ) => node.tagName === 'A' );
if ( selectedAnchor ) {
// If we selected a link, position the Link UI below the link
rect = selectedAnchor.getBoundingClientRect();
} else {
// Otherwise, position the Link UI below the cursor or text selection
rect = getRectangleFromRange( this.editor.selection.getRng() );
}
const focusPosition = this.getFocusPosition( rect );

this.setState( { formats, focusPosition, selectedNodeId: this.state.selectedNodeId + 1 } );
this.setState( { formats, selectedNodeId: this.state.selectedNodeId + 1 } );

if ( this.props.isViewportSmall ) {
let rect;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needing to be calculated at every node change event? Do we need to scroll every time on that event? Cc @mcsf.

Copy link
Member Author

Choose a reason for hiding this comment

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

It almost certainly doesn't need to be. There's definitely room for improvement here, but I considered it outside the scope of the current task.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, I like the smaller PRs :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure there's room for improvement. When we looked in #5769, there was no proper way to detect when the insertion of a character fills the whole width of a text container and forces the caret into a "new line" (a different height in the same container). 'nodechange' is definitely too generic, but per the comment, on nodechange we can work with the finalized TinyMCE instance and scroll to proper position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the same kind of reasoning we're seeing in this PR (let's manage this ourselves with our vdom abstraction so as to batch positioning writes and reads) can be applied to the caret-tracking scrolling feature.

const selectedAnchor = find( parents, ( node ) => node.tagName === 'A' );
if ( selectedAnchor ) {
// If we selected a link, position the Link UI below the link
rect = selectedAnchor.getBoundingClientRect();
} else {
// Otherwise, position the Link UI below the cursor or text selection
rect = getRectangleFromRange( this.editor.selection.getRng() );
}

// Originally called on `focusin`, that hook turned out to be
// premature. On `nodechange` we can work with the finalized TinyMCE
// instance and scroll to proper position.
Expand Down Expand Up @@ -940,7 +918,6 @@ export class RichText extends Component {
const formatToolbar = (
<FormatToolbar
selectedNodeId={ this.state.selectedNodeId }
focusPosition={ this.state.focusPosition }
formats={ this.state.formats }
onChange={ this.changeFormats }
enabledControls={ formattingControls }
Expand Down
34 changes: 34 additions & 0 deletions packages/dom/src/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,40 @@ export function getScrollContainer( node ) {
return getScrollContainer( node.parentNode );
}

/**
* Returns the closest positioned element, or null under any of the conditions
* of the offsetParent specification. Unlike offsetParent, this function is not
* limited to HTMLElement and accepts any Node (e.g. Node.TEXT_NODE).
*
* @see https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent
*
* @param {Node} node Node from which to find offset parent.
*
* @return {?Node} Offset parent.
*/
export function getOffsetParent( node ) {
// Cannot retrieve computed style or offset parent only anything other than
// an element node, so find the closest element node.
let closestElement;
while ( ( closestElement = node.parentNode ) ) {
if ( closestElement.nodeType === ELEMENT_NODE ) {
break;
}
}

if ( ! closestElement ) {
return null;
}

// If the closest element is already positioned, return it, as offsetParent
// does not otherwise consider the node itself.
if ( getComputedStyle( closestElement ).position !== 'static' ) {
return closestElement;
}

return closestElement.offsetParent;
}

/**
* Given two DOM nodes, replaces the former with the latter in the DOM.
*
Expand Down