Allow typing at the end of an inline element #4578

jameshfisher · 2021-10-08T11:22:46Z

Description

Steps to reproduce the buggy behavior:

Create a page with an inline element, or go to https://codepen.io/jameshfisher/pen/xxrXvVO
Put the cursor at the end of the inline code element
Type something

Expected behavior: If the cursor is at the end of an inline, text should be inserted at the end of the inline.

Actual behavior: Slate moves the cursor outside the inline before inserting text.

This current behavior is explicitly coded and was added in #3260. I nevertheless claim that this is a bug, or at least a misfeature in core. My expected behavior comes from:

The fact that the cursor is inside the inline. For the user, the blinking cursor is visually inside the inline, not outside it. For the developer, the cursor (i.e. editor.selection) is inside the inline, not outside it. The definition of "the cursor" is "this is where your text will go when you type". To break that behavior is really jarring.
Slate's principle that "all of its logic is implemented with a series of plugins". If the current behavior is actually desirable in some circumstance, it should be in a plugin, not core. It's harder and less elegant for me to remove the core behavior with my own plugin.
Comparison with other editors. The following editors all insert text at the end of the inline, as expected: default contenteditable, Medium, Coda.io, Google Docs, OneNote, Evernote, QuillJS, TinyMCE, EditorJS, ProseMirror. Two editors with the current buggy behavior are Notion and Dropbox Paper, but I find it hard to imagine that their current behavior on this issue is actually designed, and not just accidental.
Usability: how else is the user supposed to enter text at the end of the inline ..? The only way seems to be to insert text before the end of the inline, and then delete the rest of the inline. This is obviously horrible.

Issue
Fixes: #4524

Example

slate-inline-bug.mp4

Checks

The new code matches the existing patterns and styles.
The tests pass with yarn test.
The linter passes with yarn lint. (Fix errors with yarn fix.)
The relevant examples still work. (Run examples with yarn start.)
You've added a changeset if changing functionality. (Add one with yarn changeset add.)

This fixes ianstormtaylor#4524 Steps to reproduce the buggy behavior: * Create a page with an inline element, or go to https://codepen.io/jameshfisher/pen/xxrXvVO * Put the cursor at the end of the inline code element * Type something Expected behavior: If the cursor is at the end of an inline, text should be inserted at the end of the inline. Actual behavior: Slate moves the cursor outside the inline before inserting text. This current behavior is explicitly coded. I nevertheless claim that this is a bug, or at least a misfeature in core. My expected behavior comes from: * The fact that the cursor is inside the inline. For the user, the blinking cursor is visually inside the inline, not outside it. For the developer, the cursor (i.e. editor.selection) is inside the inline, not outside it. The definition of "the cursor" is "this is where your text will go when you type". To break that behavior is really jarring. * Slate's principle that "all of its logic is implemented with a series of plugins". If the current behavior is actually desirable in some circumstance, it should be in a plugin, not core. It's harder and less elegant for me to remove the core behavior with my own plugin. * Comparison with other editors. The following editors all insert text at the end of the inline, as expected: default contenteditable, Medium, Coda.io, Google Docs, OneNote, Evernote, QuillJS, TinyMCE, EditorJS, ProseMirror. Two editors with the current buggy behavior are Notion and Dropbox Paper, but I find it hard to imagine that their current behavior on this issue is actually designed, and not just accidental. * Usability: how else is the user supposed to enter text at the end of the inline ..? The only way seems to be to insert text before the end of the inline, and then delete the rest of the inline. This is obviously horrible.

changeset-bot · 2021-10-08T11:22:49Z

🦋 Changeset detected

Latest commit: 3bbc418

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jaked · 2021-10-09T03:49:37Z

The current behavior was added explicitly in #3260 to fix #3253, and there's still a test for it in https://github.com/ianstormtaylor/slate/blob/main/packages/slate/test/transforms/insertText/selection/inline-end.tsx

There's also some code in Editable that seems to be supposed to match the code you've removed here: https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/components/editable.tsx#L297

I think the fix in #3260 was a mistake. The issue reported in #3253 is not correct; it is possible to navigate past the end of an inline. If it's the last element in the document, you can arrow down (or drag the cursor down) to move outside the inline; or if it's not the last element, you can move one position past the end of the inline, then back again. This works in all the browsers I tested (Chrome, Safari, Firefox, all on Mac). To see this, try moving the cursor in this Slate Explorer playground and see where the <cursor/> ends up.

…havior

…special case

jameshfisher · 2021-10-12T07:38:54Z

@jaked good catch. I've updated the test and removed that code in editable.tsx. However, this seems to cause some crashes in its current state. I'll try debugging today.

jameshfisher · 2021-10-12T09:22:32Z

Here's what I've found so far.

With this PR in its current state, typing at the end of an inline causes a crash. The new text is inserted both at the end of the inline (good), and immediately after the inline element as a new DOM text node in an invalid position (bad). Slate then can't find a Slate point for this invalid new DOM text node, and so crashes.

The stuff in editable.tsx was added very recently in #3888, which lets the browser's native contenteditable events handle some kinds of input. It looks like the native event inserts the invalid new DOM text node, but I don't understand why it does that.

jameshfisher · 2021-10-12T10:27:18Z

It certainly seems to be the native input event causing the problem. If I modify onDOMBeforeInput to just use native events for everything, native Chrome has the same buggy behavior we're trying to remove: it inserts the text after the inline element. But native Firefox behaves as expected, and inserts the text at the cursor position.

I'll try to isolate this buggy Chrome behavior. I'm guessing it's related to

slate/packages/slate-react/src/components/editable.tsx

Lines 284 to 286 in 43e740c

    
           // Chrome seems to have issues correctly editing the start of nodes. 
        
           // When there is an inline element, e.g. a link, and you select 
        
           // right after it (the start of the next node).

and/or #4074 (comment)

But in short, I now believe that the special-case in editable.tsx is not there to deal with #3260; it's there to deal with buggy native events at the end of inlines.

jameshfisher · 2021-10-12T10:49:02Z

So here's the isolated Chrome bug: https://codepen.io/jameshfisher/pen/gOxbBzy

I'll submit this as a Chromium bug.

jameshfisher · 2021-10-12T11:02:38Z

Submitted Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1259100

…ed this special case" This reverts commit f9f36cd.

jameshfisher · 2021-10-12T11:11:52Z

@jaked so at the end of all that, I've left in the special-case code, and instead changed its comment - the real reason it exists is that native Chrome has (rather unbelievably) the very same bug we're trying to remove from Slate: it doesn't allow typing at the end of an <a> element.

(Honestly, the #3888 change really scares me, considering how buggy and inconsistent the native contenteditable implementations are.)

jameshfisher · 2021-10-12T11:47:14Z

The integration tests keep failing, but it looks like a flaky test to me.

dylans · 2021-10-12T12:10:28Z

The integration tests keep failing, but it looks like a flaky test to me.

Yes, I wouldn't worry about that too much.

(Honestly, the #3888 change really scares me, considering how buggy and inconsistent the native contenteditable implementations are.)

Me as well. I've been debating whether we should revert it or try to work through the various issues it has introduced.

jameshfisher · 2021-10-12T13:53:42Z

The integration tests keep failing, but it looks like a flaky test to me.

Yes, I wouldn't worry about that too much.

Okay, great. I think this PR is ready for review then.

(Honestly, the #3888 change really scares me, considering how buggy and inconsistent the native contenteditable implementations are.)

Me as well. I've been debating whether we should revert it or try to work through the various issues it has introduced.

I really, really, really appreciate the simplicity of "just re-render every time". If the goal is to make spellcheck less flickery, it feels like there must be a less radical solution (e.g. opening bug reports with buggy browser vendors, or somehow disabling spellcheck for ~1 second after each editor change). It also looks the native events doesn't help the "support IME" goal, since it currently only works on characters a-z. But I don't understand these issues well enough yet. May be worth opening a new issue to track the various issues with native character insertion ...

dylans · 2021-10-18T20:08:18Z

@jameshfisher could you please look at main after the landing of #4605 and then make sure this works fine on top of that. I'm going to release 0.67.0 now, but happy to do a 0.67.1 once this change has been verified on top of that. Thanks!

jameshfisher · 2021-10-19T06:54:26Z

Yep, will look at it today

…

On Mon, Oct 18, 2021, 21:08 Dylan Schiemann ***@***.***> wrote: @jameshfisher <https://github.com/jameshfisher> could you please look at main after the landing of #4605 <#4605> and then make sure this works fine on top of that. I'm going to release 0.67.0 now, but happy to do a 0.67.1 once this change has been verified on top of that. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4578 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABIYNX47EIOVBP7DJ63MXTUHR5DZANCNFSM5FTMSQ2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

jameshfisher · 2021-10-19T09:47:32Z

@dylans this looks good to me. Tests still pass. Examples behave as I expect. (This is clearest if you edit the "links" example to add padding and background color to the inline link. The examples suffer from a lack of non-trivial inline elements. I'm going to contribute a new example in a separate PR.)

I don't entirely understand #4605 but it looks good - perhaps less scary than previous implementation.

dylans · 2021-10-19T17:53:45Z

@jameshfisher I've landed this, but it's definitely introducing a regression. If you add a link to the end of a line, you cannot leave the link with the arrow keys (right arrow key goes to next line instead of outside the link. Going to the next line and then going back with the left arrow key does allow you to move the cursor outside the link.

jaked · 2021-10-19T18:05:21Z

@dylans this seems like expected/desired behavior given how the browser selection works, but it is sometimes annoying. I checked Notion and Google Docs and they both have the original behavior for links (edge of a link is always outside), which is also sometimes annoying!

I think @jameshfisher 's change makes sense for inlines in general, and it matches the behavior of marks. Links might be kind of a special case? Maybe applications need to be careful to leave the selection outside the link after creating / pasting a link so you don't accidentally edit the link text.

AleksandrHovhannisyan · 2021-10-19T20:30:14Z

This was perfect timing since I'm currently working on fixing this problem for inline nodes in our text editor.

Unfortunately, the new behavior is also undesirable since it forces you to hit enter and backtrack like @jaked mentioned. In my opinion, this is really no better than forcing a user to backtrack in the middle of an inline node to add the text they wanted and then delete the irrelevant bits.

Would it be possible to introduce a new placeholder node type in addition to the empty text node ({ text: '' }) that sits directly adjacent to it?

const tree = {
  type: 'paragraph',
  children: [
    { text: '...' },
    {
      type: 'hyperlink',
      href: 'google.com',
      children: [...]
    },
    { text: 'some unique placeholder' }, 
    { text: '' }
  ]
}

That way, pressing the right arrow key somehow positions the selection after that placeholder node.

Maybe Slate could detect the right arrow key at the end of an inline and insert a space after it, but not part of the inline?

Just throwing some ideas out there.

dylans · 2021-10-19T23:33:00Z

This was perfect timing since I'm currently working on fixing this problem for inline nodes in our text editor.

Unfortunately, the new behavior is also undesirable since it forces you to hit enter and backtrack like @jaked mentioned. In my opinion, this is really no better than forcing a user to backtrack in the middle of an inline node to add the text they wanted and then delete the irrelevant bits.

Would it be possible to introduce a new placeholder node type in addition to the empty text node ({ text: '' }) that sits directly adjacent to it?
const tree = {
  type: 'paragraph',
  children: [
    { text: '...' },
    {
      type: 'hyperlink',
      href: 'google.com',
      children: [...]
    },
    { text: 'some unique placeholder' }, 
    { text: '' }
  ]
}
That way, pressing the right arrow key somehow positions the selection after that placeholder node.

Maybe Slate could detect the right arrow key at the end of an inline and insert a space after it, but not part of the inline?

Just throwing some ideas out there.

It is something I do pretty regularly in our codebase and also in Plate (though this example inserts an extra empty text node) e.g. https://github.com/udecode/plate/blob/a504a1a91a6195a6e8d8c4d7ce5ec0844e9be733/packages/elements/mention/src/getMentionOnSelectItem.ts .

Historically this is something slate has not tried to do, but I think it's worth trying to solve.

jameshfisher · 2021-10-20T13:02:14Z

Slate default behavior for arrow left/right is here. It uses Transforms.move with the default unit = 'character'. There's a nice explanation of the unit parameter here.

In my application I've effectively customized the arrow left/right behavior to use unit = 'offset', at least when moving across the boundaries of some inline elements. IMO, offset is a better generic behavior because it lets the user access all possible cursor points. The unit = 'character' behavior effectively skips over some of those points, which can be frustrating and seem buggy.

But, as the Slate docs note, offset "may be counter-intuitive because the end of a Text and the beginning of the next Text might be thought of as the same position." A link is one such case. More generally, I think this rule applies if the cursor is displayed identically by the browser for both positions. This is basically when the inline element has no margin or padding, such as link elements.

We can try to improve the default behavior, but I'm sure we can't please everyone. Since Slate is schema-agnostic, and doesn't have access to CSS, Slate can't reliably know when and whether the "might be thought of as the same position" rule applies. I think this has to be decided by the application developer, for their schema and their CSS.

So instead I think we should make it easy+simple to customize this behavior. We should have an official example that shows some different inline elements, and how to modify the navigation behavior appropriately for each of them.

(Note that I also had to fight with some browser bugs when implementing this offset behavior. Perhaps Slate can help work around these browser bugs.)

AleksandrHovhannisyan · 2021-10-21T12:31:30Z

@jameshfisher I agree but also disagree. I think it's definitely worth improving the default behavior for inlines at the library level. Devs shouldn't have to customize functionality like how typing behaves in inline nodes—there should be a sensible default that everyone can use out of the box. With this change, there's no good way to break out of inline nodes once you create them, unless you press enter and backtrack. So it may actually create a more frustrating user experience unless it's customized.

I'm eagerly awaiting this feature for our app. Are there any plans to improve the typing experience before it gets released?

jameshfisher · 2021-10-21T15:15:45Z

I've opened a PR #4615 which contributes an official example of a custom inline that should clearly work with "offset" behavior: an "editable button". It shows the several hoops that I've had to jump through to get something like this working. I think we can use this official example as a medium for discussing Slate's default behavior, and how developers can customize it.

* Allow typing at the end of an inline This fixes ianstormtaylor#4524 Steps to reproduce the buggy behavior: * Create a page with an inline element, or go to https://codepen.io/jameshfisher/pen/xxrXvVO * Put the cursor at the end of the inline code element * Type something Expected behavior: If the cursor is at the end of an inline, text should be inserted at the end of the inline. Actual behavior: Slate moves the cursor outside the inline before inserting text. This current behavior is explicitly coded. I nevertheless claim that this is a bug, or at least a misfeature in core. My expected behavior comes from: * The fact that the cursor is inside the inline. For the user, the blinking cursor is visually inside the inline, not outside it. For the developer, the cursor (i.e. editor.selection) is inside the inline, not outside it. The definition of "the cursor" is "this is where your text will go when you type". To break that behavior is really jarring. * Slate's principle that "all of its logic is implemented with a series of plugins". If the current behavior is actually desirable in some circumstance, it should be in a plugin, not core. It's harder and less elegant for me to remove the core behavior with my own plugin. * Comparison with other editors. The following editors all insert text at the end of the inline, as expected: default contenteditable, Medium, Coda.io, Google Docs, OneNote, Evernote, QuillJS, TinyMCE, EditorJS, ProseMirror. Two editors with the current buggy behavior are Notion and Dropbox Paper, but I find it hard to imagine that their current behavior on this issue is actually designed, and not just accidental. * Usability: how else is the user supposed to enter text at the end of the inline ..? The only way seems to be to insert text before the end of the inline, and then delete the rest of the inline. This is obviously horrible. * add changeset * Fix test: insert at the end of an inline should _not_ have special behavior * The selection is no longer moved in insertText so we don't need this special case * fix:prettier * Revert "The selection is no longer moved in insertText so we don't need this special case" This reverts commit f9f36cd. * Explain the real reason for this special case - native browser bugs

jameshfisher added 2 commits October 8, 2021 12:13

add changeset

8609f2c

jameshfisher added 2 commits October 10, 2021 13:15

Fix test: insert at the end of an inline should _not_ have special be…

6619014

…havior

The selection is no longer moved in insertText so we don't need this …

f9f36cd

…special case

fix:prettier

d8dc562

jameshfisher added 2 commits October 12, 2021 12:03

Revert "The selection is no longer moved in insertText so we don't ne…

a13778f

…ed this special case" This reverts commit f9f36cd.

Explain the real reason for this special case - native browser bugs

560eb2a

Merge branch 'main' into allow-typing-at-end-of-inline

3bbc418

dylans approved these changes Oct 19, 2021

View reviewed changes

dylans merged commit 67badb7 into ianstormtaylor:main Oct 19, 2021

github-actions bot mentioned this pull request Oct 19, 2021

Version Packages #4610

Merged

github-actions bot mentioned this pull request Oct 20, 2021

Version Packages datalayer-externals/slate#1

Open

jameshfisher deleted the allow-typing-at-end-of-inline branch October 20, 2021 12:16

jameshfisher mentioned this pull request Oct 22, 2021

Feature request: normalizePoint method to customize cursor movement #4618

Open

AleksandrHovhannisyan mentioned this pull request Dec 1, 2021

Users cannot break out of inline nodes at the end of an editor due to #4578 #4704

Open

jameshfisher mentioned this pull request Jan 6, 2022

Windows emoji is duplicated when inserted at end of inline #4770

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow typing at the end of an inline element #4578

Allow typing at the end of an inline element #4578

jameshfisher commented Oct 8, 2021 •

edited

Loading

changeset-bot bot commented Oct 8, 2021 •

edited

Loading

jaked commented Oct 9, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

dylans commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

dylans commented Oct 18, 2021

jameshfisher commented Oct 19, 2021 via email

jameshfisher commented Oct 19, 2021

dylans commented Oct 19, 2021

jaked commented Oct 19, 2021

AleksandrHovhannisyan commented Oct 19, 2021

dylans commented Oct 19, 2021

jameshfisher commented Oct 20, 2021

AleksandrHovhannisyan commented Oct 21, 2021

jameshfisher commented Oct 21, 2021

Allow typing at the end of an inline element #4578

Allow typing at the end of an inline element #4578

Conversation

jameshfisher commented Oct 8, 2021 • edited Loading

changeset-bot bot commented Oct 8, 2021 • edited Loading

🦋 Changeset detected

jaked commented Oct 9, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

dylans commented Oct 12, 2021

jameshfisher commented Oct 12, 2021

dylans commented Oct 18, 2021

jameshfisher commented Oct 19, 2021 via email

jameshfisher commented Oct 19, 2021

dylans commented Oct 19, 2021

jaked commented Oct 19, 2021

AleksandrHovhannisyan commented Oct 19, 2021

dylans commented Oct 19, 2021

jameshfisher commented Oct 20, 2021

AleksandrHovhannisyan commented Oct 21, 2021

jameshfisher commented Oct 21, 2021

jameshfisher commented Oct 8, 2021 •

edited

Loading

changeset-bot bot commented Oct 8, 2021 •

edited

Loading