Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Wrap autocompletions as tokens
  • Loading branch information
brandonpayton committed Jun 29, 2018
commit b1a8d136a4893a2bb0eb311b92f803ab43527bd3
46 changes: 34 additions & 12 deletions components/autocomplete/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,22 +231,44 @@ export class Autocomplete extends Component {
this.node = node;
}

insertCompletion( range, replacement ) {
const container = document.createElement( 'div' );
container.innerHTML = renderToString( replacement );
while ( container.firstChild ) {
const child = container.firstChild;
container.removeChild( child );
range.insertNode( child );
range.setStartAfter( child );
}
insertCompletion( range, replacement, completerName ) {
// Wrap completions so we can treat them as tokens with editing boundaries.
const tokenWrapper = document.createElement( 'span' );
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a native tinymce API for this?

Copy link
Member

Choose a reason for hiding this comment

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

There are TinyMCE APIs for this, but I'm guessing this component shouldn't depend on TinyMCE? Only the RichText component is aware of it. This is similar to writing flow which has tried to avoid using TinyMCE APIs. If we want we could pass the editor instance though.

tokenWrapper.classList.add( 'autocomplete-token' );
tokenWrapper.classList.add( `autocomplete-token-${ completerName }` );

tokenWrapper.innerHTML = renderToString( replacement );

range.insertNode( tokenWrapper );
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should move this to dom utils and keep all range logic there...

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 feel hesitant to move this to the dom utils because I dislike the   compromise so much :-P

But if we were to move this to dom utils, what do you imagine we'd create there? Something like this?

// Please feel free to suggest improved name.
export function replaceRangeWithToken( range : Range, value : <string|WPElement>, tokenClasses : Array<string> ) {
  const tokenWrapper = document.createElement( 'span' );
  tokenClasses.forEach( ( c ) => tokenWrapper.classList.add( c ) );
  tokenWrapper.innerHTML = renderToString( replacement );
  
  range.insertNode( tokenWrapper );
  range.setStartAfter( tokenWrapper );
  range.deleteContents();
  
  // <comment explaining why nbsp>
  tokenWrapper.parentNode.insertBefore(
  	document.createTextNode( '\u00A0' ),
  	tokenWrapper.nextSibling
  );
  
  const selection = window.getSelection();
  selection.removeAllRanges();
  
  const newCursorPosition = document.createRange();
  newCursorPosition.setStartAfter( tokenWrapper.nextSibling );
  selection.addRange( newCursorPosition );
}

I feel like the above may be too specific to be valuable for reuse, but it would be nice to keep Range-related logic in the dom utils.

range.setStartAfter( tokenWrapper );
range.deleteContents();

/*
* Add non-breaking space after a completion because:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean, though, that there will never be a break in between a mention and the next word? This could result in awkward-looking text flows.

For example, this mention at the end of the line causes stuff @editor done to go onto a new line:

screen shot 2018-05-07 at 11 29 25

Here's how it would look without non-breaking spaces:

screen shot 2018-05-07 at 11 32 08

Maybe it's a compromise we're OK with. Just checking 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

@noisysocks It is definitely a compromise. In my testing, I was unable to add a regular space and programmatically place the cursor afterward. This appears to be the issue or at least related:
https://bugs.chromium.org/p/chromium/issues/detail?id=428313

I thought about using zero-width non-breaking spaces as TinyMCE seems to do to move the cursor around inline boundaries, making it easier to specifically target and edit things like link text. But the character is not always removed by the editor, and it's invisible nature can cause problems. For example, in Firefox, when I backspace from one paragraph block into another with a trailing text node containing a zero-width non-breaking space, the cursor is not placed at the end of the text but rather one character before the end.

Inserting a visible non-breaking space is the best I've come up with to place the cursor and have editing work as expected after inserting an autocomplete token. I'm not totally OK with it but figured we could live with it.

I'd love to hear other ideas though.

* 1. If the inserted token is the last child in Chrome 66 and desktop Safari 11,
* we can set the cursor after the token and receive user input,
* but if the input isn't preceded by a space, the user may not place the
* cursor within the text by clicking. Adding a subsequent non-breaking space
* avoids this issue. A regular space is insufficient.
* 2. It seems reasonable to separate a token from subsequent text with a space.
*/
tokenWrapper.parentNode.insertBefore(
document.createTextNode( '\u00A0' ),
tokenWrapper.nextSibling
);

const selection = window.getSelection();
selection.removeAllRanges();

const newCursorPosition = document.createRange();
newCursorPosition.setStartAfter( tokenWrapper.nextSibling );
selection.addRange( newCursorPosition );
}

select( option ) {
const { onReplace } = this.props;
const { open, range, query } = this.state;
const { getOptionCompletion } = open || {};
const { getOptionCompletion, name: completerName } = open || {};

if ( option.isDisabled ) {
return;
Expand All @@ -265,14 +287,14 @@ export class Autocomplete extends Component {
if ( 'replace' === action ) {
onReplace( [ value ] );
} else if ( 'insert-at-caret' === action ) {
this.insertCompletion( range, value );
this.insertCompletion( range, value, completerName );
} else if ( 'backcompat' === action ) {
// NOTE: This block should be removed once we no longer support the old completer interface.
const onSelect = value;
const deprecatedOptionObject = option.value;
const selectionResult = onSelect( deprecatedOptionObject.value, range, query );
if ( selectionResult !== undefined ) {
this.insertCompletion( range, selectionResult );
this.insertCompletion( range, selectionResult, completerName );
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions components/autocomplete/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,7 @@
@include button-style__hover;
}
}

.autocomplete-token[data-mce-selected] {
Copy link
Member

Choose a reason for hiding this comment

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

cc @jasmussen for color choice and whether we should show the background at all times, not just on selection (similar to the code inline format).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @karmatosed is looking at this now. My thoughts are these:

  • We should definitely leverage, if we can, the inline boundaries for tokens as well.
  • Inline boundaries, like bold, italic, links, should feel "ephemeral" and disappear when you're not interacting with the boundary. Subtle colors and no borders make them feel ephemeral.
  • The tokens should feel more substantial, and should probably keep their boundaries even when you're not interacting with them. A subtle background plus border can help make them feel like "chips", which I think would make them feel more substantial, which seems a good thing.

background-color: $blue-medium-highlight;
}
2 changes: 1 addition & 1 deletion editor/components/rich-text/tinymce.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export default class TinyMCE extends Component {
browser_spellcheck: true,
entity_encoding: 'raw',
convert_urls: false,
inline_boundaries_selector: 'a[href],code,b,i,strong,em,del,ins,sup,sub',
inline_boundaries_selector: 'a[href],code,b,i,strong,em,del,ins,sup,sub,.autocomplete-token',
plugins: [],
formats: {
strikethrough: { inline: 'del' },
Expand Down
79 changes: 79 additions & 0 deletions test/e2e/specs/autocompletion.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* Internal dependencies
*/
import '../support/bootstrap';
import { newPost, newDesktopBrowserPage } from '../support/utils';

describe( 'autocompletion', () => {
beforeAll( async () => {
await newDesktopBrowserPage();
await newPost();
} );

it( 'adds a token followed by a non-breaking space and the cursor', async () => {
await page.click( '.editor-default-block-appender' );

const contentEditableHandle = (
await page.evaluateHandle( () => document.activeElement )
);

await contentEditableHandle.asElement().type( '@' );
const optionNode = await page.waitForSelector( '.components-autocomplete__result', { timeout: 10000 } );
optionNode.click();

// Wait for content update.
await page.waitForFunction(
( contentEditableNode ) => contentEditableNode.textContent.length > 0,
{ timeout: 1000 },
contentEditableHandle
);

// Confirm we contain the selection.
expect( await page.evaluate(
( contentEditableNode ) => {
const { anchorNode, isCollapsed } = window.getSelection();
return contentEditableNode.contains( anchorNode ) && isCollapsed;
},
contentEditableHandle
) ).toBeTruthy();

// Confirm the expected content.
expect( await page.evaluate(
( contentEditableNode ) => contentEditableNode.textContent,
contentEditableHandle
) ).toMatch( /^@\w+\u00A0$/ );

// Selection is placed after a single-space text node.
const selectionAnchorHandle = await page.evaluateHandle( () => window.getSelection().anchorNode );
expect( await page.evaluate(
( anchorNode ) => anchorNode.nodeType === window.Node.TEXT_NODE,
selectionAnchorHandle
) ).toBeTruthy();
const nonBreakingSpace = '\u00A0';
expect( await page.evaluate(
( anchorNode ) => anchorNode.nodeValue,
selectionAnchorHandle
) ).toBe( nonBreakingSpace );
expect( await page.evaluate(
() => window.getSelection().anchorOffset
) ).toBe( nonBreakingSpace.length );

// The autocomplete token precedes the space.
const tokenHandle = await page.evaluateHandle(
( anchorNode ) => anchorNode.previousSibling,
selectionAnchorHandle
);
expect( await page.evaluate(
( tokenNode ) => tokenNode.nodeType === window.Node.ELEMENT_NODE,
tokenHandle
) ).toBeTruthy();
expect( await page.evaluate(
( tokenNode ) => tokenNode.classList.contains( 'autocomplete-token' ),
tokenHandle
) ).toBeTruthy();
expect( await page.evaluate(
( tokenNode ) => /^@\w+$/.test( tokenNode.textContent ),
tokenHandle
) ).toBeTruthy();
} );
} );