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

Links: do not reposition container on selection change #14921

Closed
wants to merge 5 commits into from
Closed

Links: do not reposition container on selection change #14921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bcf0dd2
Links: do not reposition container on selection change
ellatrix Apr 11, 2019
36eac5e
Use caret position when adding link
ellatrix Apr 11, 2019
d7cc93e
WIP
ellatrix Apr 11, 2019
6a516e0
Fix annotations
ellatrix Apr 11, 2019
f44a316
Fix initial selection
ellatrix Apr 12, 2019
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
65 changes: 43 additions & 22 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
omit,
pickBy,
} from 'lodash';
import memize from 'memize';

/**
* WordPress dependencies
Expand Down Expand Up @@ -40,7 +39,6 @@ import {
__UNSTABLE_LINE_SEPARATOR as LINE_SEPARATOR,
__unstableIndentListItems as indentListItems,
__unstableGetActiveFormats as getActiveFormats,
__unstableUpdateFormats as updateFormats,
} from '@wordpress/rich-text';
import { decodeEntities } from '@wordpress/html-entities';
import { withFilters, IsolatedEventContainer } from '@wordpress/components';
Expand Down Expand Up @@ -147,12 +145,8 @@ export class RichText extends Component {
this.handleHorizontalNavigation = this.handleHorizontalNavigation.bind( this );
this.onPointerDown = this.onPointerDown.bind( this );

this.formatToValue = memize(
this.formatToValue.bind( this ),
{ maxSize: 1 }
);

this.savedContent = value;
this.rawValue = this.formatToValue( value );
this.patterns = getPatterns( {
onReplace,
valueToFormat: this.valueToFormat,
Expand Down Expand Up @@ -199,7 +193,7 @@ export class RichText extends Component {
* @return {Object} The current record (value and selection).
*/
getRecord() {
const { formats, replacements, text } = this.formatToValue( this.props.value );
const { formats, replacements, text } = this.rawValue;
const { start, end, activeFormats } = this.state;

return { formats, replacements, text, start, end, activeFormats };
Expand Down Expand Up @@ -231,7 +225,7 @@ export class RichText extends Component {
}

isEmpty() {
return isEmpty( this.formatToValue( this.props.value ) );
return isEmpty( this.rawValue );
}

/**
Expand Down Expand Up @@ -380,6 +374,7 @@ export class RichText extends Component {
}

this.recalculateBoundaryStyle();
this.onSelectionChange();

document.addEventListener( 'selectionchange', this.onSelectionChange );
}
Expand Down Expand Up @@ -419,16 +414,33 @@ export class RichText extends Component {
}
}

const record = this.getRecord();
const { activeFormats = [], start, end } = record;
const formats = createPrepareEditableTree( this.props )( record );
const value = this.createRecord();
const { activeFormats = [], start } = this.state;

let newFormats;

// Update the formats between the last and new caret position.
const change = updateFormats( {
value,
start,
end: value.start,
formats: activeFormats,
} );
if ( value.start <= start ) { // Deletion
const beforeFormats = formats.slice( 0, value.start );
const afterFormats = formats.slice( end );

newFormats = beforeFormats.concat( afterFormats );
} else { // Insertion
const length = value.start - start;
const formatsToInsert = Array.from( Array( length ), () => activeFormats );
const beforeFormats = formats.slice( 0, start );
const afterFormats = formats.slice( end );

newFormats = beforeFormats.concat( formatsToInsert, afterFormats );
}

const change = {
...value,
formats: newFormats,
activeFormats,
};

this.onChange( change, { withoutHistory: true } );

Expand Down Expand Up @@ -459,12 +471,18 @@ export class RichText extends Component {
* Handles the `selectionchange` event: sync the selection to local state.
*/
onSelectionChange() {
const value = this.createRecord();
const { start, end } = value;
const { start, end } = this.createRecord();

if ( start !== this.state.start || end !== this.state.end ) {
const { isCaretWithinFormattedText } = this.props;
const activeFormats = getActiveFormats( value );
const value = this.getRecord();
const newValue = {
...value,
start,
end,
};
delete newValue.activeFormats;
const activeFormats = getActiveFormats( newValue );
Copy link
Member

Choose a reason for hiding this comment

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

If we're working with a shallow clone, seems we might as well assign activeFormats directly, rather than create another shallow clone below?

const newValue = {
	...value,
	activeFormats: getActiveFormats( newValue ),
	start,
	end,
};


if ( ! isCaretWithinFormattedText && activeFormats.length ) {
this.props.onEnterFormattedText();
Expand All @@ -473,7 +491,7 @@ export class RichText extends Component {
}

this.setState( { start, end, activeFormats } );
this.applyRecord( { ...value, activeFormats }, { domOnly: true } );
this.applyRecord( { ...newValue, activeFormats }, { domOnly: true } );

if ( activeFormats.length > 0 ) {
this.recalculateBoundaryStyle();
Expand Down Expand Up @@ -517,6 +535,7 @@ export class RichText extends Component {
changeHandler( record.formats, record.text );
} );

this.rawValue = record;
this.savedContent = this.valueToFormat( record );
this.props.onChange( this.savedContent );
this.setState( { start, end, activeFormats } );
Expand Down Expand Up @@ -733,8 +752,8 @@ export class RichText extends Component {
* @param {SyntheticEvent} event A synthetic keyboard event.
*/
handleHorizontalNavigation( event ) {
const value = this.createRecord();
const { formats, text, start, end } = value;
const value = this.getRecord();
const { text, start, end } = value;
const { activeFormats = [] } = this.state;
const collapsed = isCollapsed( value );
const isReverse = event.keyCode === LEFT;
Expand Down Expand Up @@ -763,6 +782,7 @@ export class RichText extends Component {
// In all other cases, prevent default behaviour.
event.preventDefault();

const formats = createPrepareEditableTree( this.props )( value );
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this change is here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll comment, it's to make sure the boundaries work for annotations.

const formatsBefore = formats[ start - 1 ] || [];
const formatsAfter = formats[ start ] || [];

Expand Down Expand Up @@ -915,6 +935,7 @@ export class RichText extends Component {
}

this.applyRecord( record );
this.rawValue = record;
this.savedContent = value;
}

Expand Down
35 changes: 31 additions & 4 deletions packages/components/src/positioned-at-selection/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,12 @@ import { getOffsetParent, getRectangleFromRange } from '@wordpress/dom';
* relative to the bottom-center of the current selection. Includes `top` and
* `left` style properties.
*
* @param {string} selector Use the position of the closest element that matches
* the selector instead of the position of the caret.
*
* @return {Object} Style object.
*/
function getCurrentCaretPositionStyle() {
function getCurrentCaretPositionStyle( selector ) {
const selection = window.getSelection();

// Unlikely, but in the case there is no selection, return empty styles so
Expand All @@ -20,8 +23,32 @@ function getCurrentCaretPositionStyle() {
return {};
}

const range = selection.getRangeAt( 0 );

// Get position relative viewport.
const rect = getRectangleFromRange( selection.getRangeAt( 0 ) );
let rect;

if ( selector ) {
let element = range.startContainer;

// If the caret is right before the element, select the next element.
element = element.nextElementSibling || element;

while ( element.nodeType !== window.Node.ELEMENT_NODE ) {
element = element.parentNode;
}

const closest = element.closest( selector );

if ( closest ) {
rect = closest.getBoundingClientRect();
}
}

if ( ! rect ) {
rect = getRectangleFromRange( range );
}

let top = rect.top + rect.height;
let left = rect.left + ( rect.width / 2 );

Expand All @@ -44,11 +71,11 @@ function getCurrentCaretPositionStyle() {
* @type {WPComponent}
*/
export default class PositionedAtSelection extends Component {
constructor() {
constructor( { selector } ) {
super( ...arguments );

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

Expand Down
29 changes: 24 additions & 5 deletions packages/format-library/src/link/inline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
applyFormat,
getTextContent,
slice,
getActiveFormat,
} from '@wordpress/rich-text';
import { URLInput, URLPopover } from '@wordpress/block-editor';

Expand Down Expand Up @@ -108,23 +109,36 @@ class InlineLinkUI extends Component {
this.state = {
opensInNewWindow: false,
inputValue: '',
key: 0,
};
}

static getDerivedStateFromProps( props, state ) {
const { activeAttributes: { url, target } } = props;
const { activeAttributes: { url, target }, value } = props;
const opensInNewWindow = target === '_blank';
const activeFormat = getActiveFormat( value, 'core/link' );

const newState = {};

if ( ! isShowingInput( props, state ) ) {
if ( url !== state.inputValue ) {
return { inputValue: url };
newState.inputValue = url;
}

if ( opensInNewWindow !== state.opensInNewWindow ) {
return { opensInNewWindow };
newState.opensInNewWindow = opensInNewWindow;
}
}

if ( activeFormat && activeFormat !== state.activeFormat ) {
newState.activeFormat = activeFormat;
newState.key = state.key + 1;
}

if ( Object.keys( newState ).length ) {
return newState;
}

return null;
}

Expand Down Expand Up @@ -211,7 +225,7 @@ class InlineLinkUI extends Component {
}

render() {
const { isActive, activeAttributes: { url }, addingLink, value } = this.props;
const { isActive, activeAttributes: { url }, addingLink } = this.props;

if ( ! isActive && ! addingLink ) {
return null;
Expand All @@ -222,7 +236,12 @@ class InlineLinkUI extends Component {

return (
<PositionedAtSelection
key={ `${ value.start }${ value.end }` /* Used to force rerender on selection change */ }
Copy link
Member

@aduth aduth Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Possible dumb question: Couldn't a minimal solution just be to remove key (and make no other changes)?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if you move the caret from one link to another?

Copy link
Member

@aduth aduth Apr 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What if you move the caret from one link to another?

That's true. I wonder then if it could either act more like the Autocomplete component in rendering Popover with a getAnchorRect that updates with the selection (but keeps the same element so not to unmount and re-trigger animation), or if PositionedAtSelection should update its styling in response to selection changes rather than once at mount. Both of these would still update the position on each selection change, but at least would avoid the repeated animations.

I'm not totally sure the ramifications of either of these alternatives. Just trying to consider in terms of the least potential for impact.

// When adding a new link, set the container at the caret,
// otherwise set it at the link element.
selector={ addingLink ? null : 'a' }
// Since the key cannot be the format object, we have to keep it
// in the state and bump it when the object reference changes.
key={ this.state.key }
>
<URLPopover
onClickOutside={ this.onClickOutside }
Expand Down
1 change: 0 additions & 1 deletion packages/rich-text/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,4 @@ export { unregisterFormatType } from './unregister-format-type';
export { indentListItems as __unstableIndentListItems } from './indent-list-items';
export { outdentListItems as __unstableOutdentListItems } from './outdent-list-items';
export { changeListType as __unstableChangeListType } from './change-list-type';
export { updateFormats as __unstableUpdateFormats } from './update-formats';
export { getActiveFormats as __unstableGetActiveFormats } from './get-active-formats';
55 changes: 0 additions & 55 deletions packages/rich-text/src/test/update-formats.js
View file
Open in desktop

This file was deleted.

Loading