Treat autocompletions as tokens with editing boundaries

[brandonpayton]

Description

This PR updates the <Autocomplete> component to insert autocompletions as tokens.

Completions consisting entirely of text are inserted as styled, editable tokens. This satisfies the common completer use cases of user mentions (@-user), tags (#-tag), and cross posting site mentions (+site).

Here's an example of editable tokens:

Completions containing HTML are inserted as non-editable tokens. This supports the creation of completers that insert arbitrary HTML content.

Here's an example of a non-editable token created from this completer:

Closes #6573.

How has this been tested?

Tested manually in Chrome, Firefox, and Safari on macOS and in IE11 and Edge on Windows.

I also added an e2e test for basic token insertion.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[brandonpayton]

Hi @iseulde, it looks like we have what we need now. @azaozz chimed in about this, and I ended up adding a non-breaking space after inserted tokens, which appears to work around this issue.

[ellatrix]

So the user is not allowed to edit e.g. a mention? Why not? This PR does not clarify why this change is needed.

[noisysocks]

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:

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

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

[brandonpayton]

@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.

[brandonpayton]

So the user is not allowed to edit e.g. a mention? Why not? This PR does not clarify why this change is needed.

Thanks for asking, @iseulde. I should have included my reasoning. I added it to the PR description and would love to hear what you think.

[brandonpayton]

I spoke with Matías, and he'd like these to be editable. I'm working on an update now.

[brandonpayton]

I updated this PR to make the tokens editable with inline boundaries, a generic autocomplete-token class for general styling, and an autocomplete-token-<completer-name> class for styling tokens from specific completers.

[brandonpayton]

@karmatosed, is there a particular style you'd like to see applied to autocomplete tokens either in general or specifically (like styles for the tokens inserted for @ user mentions)?

[karmatosed]

@karmatosed, is there a particular style you'd like to see applied to autocomplete tokens either in general or specifically (like styles for the tokens inserted for @ user mentions)?

@brandonpayton do you mean when added into content? If that is right then just let them be styled as normal. I think if they link to user profiles or maybe even later profiles they can have link styling, however unless a link they should just be a normal style.

[brandonpayton]

When discussing integrating Gutenberg with P2, @mtias mentioned that it might be good to highlight xpost tokens like:

My impression is that he wanted other tokens such as user mentions highlighted in some way. It's not a blocker for this PR though. I'm adding classes to the tokens, and we can add styles for them later. Based on your input, I'll leave them unstyled for now. Thank you!

[ellatrix]

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

[brandonpayton]

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.

[ellatrix]

Would this be a good candidate for an e2e test?

[brandonpayton]

Would this be a good candidate for an e2e test?

Seems like an excellent candidate. Thanks, I'll work on one.

[brandonpayton]

I added an e2e test for token insertion, but it is really clunky. I'm going to give my mind a break and then return to see if I can clean it up.

[brandonpayton]

I updated the e2e test. It's still not nearly as readable as I'd like, but I think it's better.

[mtias]

There seems to be a bug where you edit an existing mention, delete a letter or two, get the auto-suggestion popover, select the name again and you end up with two:

[mtias]

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

[ellatrix]

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.

[mtias]

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).

[jasmussen]

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.

[brandonpayton]

Thanks, @mtias.

There seems to be a bug where you edit an existing mention, delete a letter or two, get the auto-suggestion popover, select the name again and you end up with two

My mind also sent this to me over the weekend, realizing I hadn't added any logic to prevent it.

I believe the preferred behavior is that the completion menu displays when editing a token but only replaces the existing token. I'll work on a fix like this, but please let me know if you'd like a different behavior.

[brandonpayton]

There are some challenges to displaying the completion menu when editing tokens since completions may have arbitrary content and structure. Here are some reasons:

1. Completions are not required to include their triggerPrefix (e.g., how user mentions include the '@' character). For example, here's an abbreviations completer that doesn't include its trigger prefix in the completion:
   https://gist.github.com/brandonpayton/01b75c55ce4d329d534b1d3a68dc728f
2. The content of a completion is not guaranteed to be a query for the option used to generate the completion. In other words, if the completion text is different than what the user typed, we can't display the same list of options when editing. For example, a synonym completer would take one word and allow selection of a synonym X, but when editing, the original word is lost, and instead, we might propose synonyms for X.

There are straightforward cases like user mentions (@-user), tags (#-tag), and xposts (+blogname). They contain their trigger prefix, and the completion is also a valid, specific query for the option that generated the completion. We can handle these cases and know how to show a completion menu when editing.

Other cases may require more work to handle due to the challenges mentioned above.

Some brainstorming on solution or workaround:

• Store the query used to generate a completion and use that somehow when editing.
• Allow marking completers with an editableCompletions flag to indicate whether completions can be edited. Document the requirements for a truly editable completion. The straightforward cases could be editable, and the more complicated cases could be non-editable.
• Update the completer design to only support the simple cases.

My intent is not to expand our scope but to consider what is currently possible with today's completer design so we can properly handle editing.

@mtias, do you have any thoughts on this? My impression is that you may have a simpler vision for completers than what I described above.

[brandonpayton]

I pushed 0c0f470 to stop the completion menu from lingering after deleting a token with backspace.

The remaining bug is incorrectly inserting tokens inside formatting as mentioned here.

[brandonpayton]

The incorrect insertion placement appears to be caused by a DOM Range referring to a text node that has been replaced. The range first refers to the text node, then later refers to the text node's parent with its start and end offsets zeroed. I am not yet sure how to fix this but am working on it.

[brandonpayton]

The incorrect insertion placement appears to be caused by a DOM Range referring to a text node that has been replaced. The range first refers to the text node, then later refers to the text node's parent with its start and end offsets zeroed. I am not yet sure how to fix this but am working on it.

It looks like the solution needs to be identifying the range on-demand since the editor occasionally replaces text nodes and changes our saved range. I likely need to wait until Monday to make this change but plan to work on it first thing.

[brandonpayton]

Hi @iseulde, I believe this is ready for another review.

Undo remains a bit wonky related to your comment here:

I think all these DOM manipulations need to we wrapped in editor.undoManager.transact so that we are sure this is one history record. Becomes harder to do though as this component is not aware of the TinyMCE editor instance.

Do you see a reasonable way to fix this with the current Autocomplete component design? It definitely feels like there should be more direct integration with TinyMCE rather than direct DOM manipulation here. Autocomplete is written in such a way that it assumes contenteditable anyway.

It would also be good to get your feedback on my responses to your comments here and here.

Thank you for your time on this.

cc @mtias in case you want to comment as well.

[brandonpayton]

I played with adding an optional transactInsertion prop to <Autocomplete> to facilitate integration with an undo stack. It seems to work well, but I need to test a bit more and document before pushing.

[brandonpayton]

I added a change to effectively wrap insertions with editor.undoManager.transact. The undo experience is better but not perfect. Perhaps it is good enough for a first version of autocomplete tokens.

[brandonpayton]

Hi @azaozz, would you be up for reviewing my changes here? Reading the PR description and conversation starting here should be a sufficient background. I understand there may not be bandwidth at the moment but wanted to ask. Thanks for your consideration. :)

[brandonpayton]

Hi @mtias, I took some time to review @iseulde's WIP PR (#7890) that explores formatting based on pattern matching among other things. I think it is a stronger, less error-prone approach than the one in this PR. Since I've had a hard time getting further review on this PR and the fact that it has grown to accommodate fixes for various contenteditable issues, I'm wondering:
Would you like to defer this feature to that PR, or should I continue working to get this merged? As for the state of this PR, it is waiting on further review and apparently has a code coverage regression that needs fixed.

[brandonpayton]

I'm closing this as it would be stronger and cleaner to implement autocomplete tokens based on @iseulde's rich text structure updates.