fix: Broken text replacement feature (macOS-only) #5261

rfgamaral · 2024-06-21T12:38:37Z

Changes Overview

Implementation Approach

This input rules feature currently works by handling the handleTextInput event, which is fired whenever the user directly inputs text. However, it's important to note that this handler is called before the input is applied, which is basically the source of the issue. Because of that, when a macOS text replacement happens, the positions used by the logic that handles input rules, don't match the editor state, and the editor breaks. This is an oversimplified version, but if you are interested to know more, I've written about it here.

The solution implemented in this PR's patch replaces handleTextInput with handleDOMEvents for input rules, which allows us to handle the keyup event. Such event is better suitable here because it will fire after the user directly inputs text, which means the editor state will be as updated as it can be, and all position calculations will match that state.

Testing Done

I've patched our app using Tiptap with the exact changes in this PR, and our millions of users have been using it for 3 weeks now, and we haven't received any tickets regarding editor issues since then. I'd say this change was a success.

Verification Steps

See the reproduction steps in the referenced issue that this PR fixes.

Additional Notes

Checklist

I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
My changes do not break the library.
I have followed the project guidelines.
I have fixed any lint issues.

netlify · 2024-06-21T12:38:52Z

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	`e918404`
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/66c703ea36e14d000846e8a2
😎 Deploy Preview	https://deploy-preview-5261--tiptap-embed.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rfgamaral · 2024-06-21T17:33:02Z

@bdbch A lot of tests are breaking because the new implementation doesn't seem to play well with cy.type(). Tried to replace with realType() (ref), and while I was able to fix some of them, not all of them were easily fixable with that library. Not exactly sure how to handle this, do you think you can have a look?

nperez0111 · 2024-06-25T14:59:48Z

Hm. This change does really feel like an edge case and I'm not super confident in the change. Could we not account for the positions to make sure that they are out of range?

rfgamaral · 2024-07-29T09:16:06Z

This change does really feel like an edge case and I'm not super confident in the change.

For what it's worth, we've been using this change on Todoist for almost 2 months now, and so far we haven't had any bug reports related to the change. That said, I'd be happy if someone else could have a look and find a different solution, but personally, I have no idea how that looks like (I've already spent a lot of time on it, and this was my best shot).

changeset-bot · 2024-08-13T10:35:49Z

⚠️ No Changeset found

Latest commit: e918404

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

rfgamaral · 2024-08-21T08:58:27Z

@nperez0111 Any thoughts on how to better fix this issue?

nperez0111 · 2024-08-21T09:52:07Z

Hey @rfgamaral been looking at this now,

So, I think that we should be fixing the root cause instead of trying to make a workaround like this.

The root cause is that what got inputted & what exists in the editor are different. & I'm unsure that the macos text replacement feature let's you really do anything about this.

I'm not sure what a proper resolution is for this other than what you've come up with

Separately, I took a look at what remirror does (they seem to just not apply the text in this situation), I noticed that they are trying to preserve the initial stored marks https://github.com/remirror/remirror/blob/b3c8b02f7562afb7de08b4308a3302a500386d33/packages/remirror__core-utils/src/prosemirror-rules.ts#L213 and I think that we should be doing that too actually.

rfgamaral · 2024-08-21T09:56:52Z

Separately, I took a look at what remirror does (they seem to just not apply the text in this situation), I noticed that they are trying to preserve the initial stored marks https://github.com/remirror/remirror/blob/b3c8b02f7562afb7de08b4308a3302a500386d33/packages/remirror__core-utils/src/prosemirror-rules.ts#L213 and I think that we should be doing that too actually.

But going that route, the macOS text replacement feature will cease to work, right?

nperez0111 · 2024-08-21T10:26:21Z

But going that route, the macOS text replacement feature will cease to work, right?

I think the storedMarks is completely separate & orthogonal to this, I just saw it in trying to understand how they implemented it.

In their editor, the text will replace, but not match the mark, which I honestly think is acceptable. I think that the main goal here should be to not crash the editor and a secondary goal can be to get it to be marked up properly.

The main issue that I have with your solution is:

I'm unsure how it will work with someone using IME, as I'm unsure if that triggers keyup.
it is pretty obscure why it needs to be handled like that & I don't like that it gates on there being a single character inputted, I'm unsure how that would work with other languages

rfgamaral · 2024-08-21T11:26:24Z

I think that the main goal here should be to not crash the editor and a secondary goal can be to get it to be marked up properly.

I can understand that, although, to be fair, the editor doesn't crash per se 😅, it continues to work despite the error on the console.

The main issue that I have with your solution is

For what it's worth, I can tell you that we've been using this workaround for a couple of months now, and we have a huge user base where some users use IMEs, and we also support lots of different languages. So far, we haven't had anyone complaining about issues in the editor related to this particular change.

I'm unsure how it will work with someone using IME, as I'm unsure if that triggers keyup.

Firefox seems to fire it as expected (ref), and I'm sure Chromium-based browsers will as well.

it is pretty obscure why it needs to be handled like that

I don't think it's that obscure... I mean, we've reached the conclusion that the main issue is "what got inputted & what exists in the editor are different", and that's an issue caused by handling input rules with handleTextInput which is called before the input is applied to the editor state. My solution only changes the handling of input rules to be called after the input is applied to the editor state. Why do you think that's obscure?

If we go to the ProseMirror source (ref), we can see that handleTextInput is called within keypress, and I've only changed this to be handled on keyup. If ProseMirror had implemented a handleTextInputBefore (keypress) and handleTextInputAfter (keyup), it would've been just a matter of changing the name of the handler.

nperez0111 · 2024-08-21T12:07:38Z

Alright, I'm convinced by your argument.

I do need for these tests to pass though.

@bdbch you know cypress better than I do, can we get this to work? Also would appreciate your input on what was discussed here

rfgamaral · 2024-08-21T15:00:55Z

Alright, I'm convinced by your argument.

I wasn't trying to convince you per se 😅, just sharing my point of view. Ultimately, this is your call, of course 👍

rfgamaral · 2024-08-21T16:11:16Z

@nperez0111 The test pipeline is still running, but I should've fixed all Cypress tests. It required the usage of cypress-real-events for typing instead of using Cypress's default type (which are simulated and not real events, thus not all handlers are called correctly when expected).

nperez0111 · 2024-08-21T17:12:28Z

@nperez0111 The test pipeline is still running, but I should've fixed all Cypress tests. It required the usage of cypress-real-events for typing instead of using Cypress's default type (which are simulated and not real events, thus not all handlers are called correctly when expected).

Thanks for this, didn't realize that you had a library in mind that worked for this.

rfgamaral · 2024-08-21T17:55:25Z

Thanks for this, didn't realize that you had a library in mind that worked for this.

It's what we also use internally to solve issues like this (i.e. when .type from Cypress is insufficient).

rfgamaral · 2024-08-22T09:34:59Z

@nperez0111 @bdbch @svenadlung Finally got all tests to pass, feel free to review.

nperez0111 · 2024-08-22T10:07:37Z

tests/cypress/support/commands.js

@@ -95,3 +97,11 @@ Cypress.Commands.add(
    return subject
  },
 )
+
+Cypress.Commands.add('resetEditor', () => {


nperez0111 · 2024-08-22T10:08:50Z

demos/src/Examples/InteractivityComponentContent/React/index.spec.js

+    cy.get('.tiptap .react-component .content div')
+      .realType('Hello World! This is **bold**.')
+    cy.get('.tiptap .react-component .content div')


Just so I understand better, does realType not support chaining or is this a timing thing?

I'm no Cypress expert but:

I initially tried with chaining, but it didn't work

All our internal tests don't chain the .real* calls

Examples on the official repo do use chaining

But like I said, I couldn't make it work...

nperez0111 · 2024-08-22T10:09:13Z

demos/src/Examples/InteractivityComponentContent/React/index.spec.js

@@ -4,44 +4,47 @@ context('/src/Examples/InteractivityComponentContent/React/', () => {
  })

  it('should have a working tiptap instance', () => {
-    cy.get('.ProseMirror').then(([{ editor }]) => {
+    cy.get('.tiptap').then(([{ editor }]) => {


Appreciate you adding consistency here

nperez0111

This is a fantastic contribution @rfgamaral I really appreciate the work you put into this

bdbch

lgtm! ty @rfgamaral

Nantris · 2024-09-07T22:51:04Z

@rfgamaral I'm sorry to report this change breaks our existing input rules, and further breaks undo behavior for our apparently still-functional input rules. See #5598

#5261)" This reverts commit 88e310b.

nperez0111 · 2024-09-17T13:34:52Z

This was not released with 2.7 due to the potential breaking change

Nantris · 2024-09-17T20:03:49Z

Just FYI I noticed this is listed in the 2.7.0 change list despite not being included.

Sorry again to have been the bearer of bad news. It's a worthwhile goal to get InputRules working with the text replacement feature on macOS.

rfgamaral requested review from bdbch and svenadlung as code owners June 21, 2024 12:38

rfgamaral force-pushed the macos-text-replacement branch from 5e352d2 to b6ed732 Compare August 13, 2024 10:35

rfgamaral force-pushed the macos-text-replacement branch from b6ed732 to eb836a7 Compare August 20, 2024 16:08

nperez0111 changed the base branch from main to develop August 21, 2024 06:48

rfgamaral force-pushed the macos-text-replacement branch 2 times, most recently from 119e4a4 to 07980c4 Compare August 21, 2024 08:57

rfgamaral force-pushed the macos-text-replacement branch 2 times, most recently from 9fcd057 to b3d9ba0 Compare August 21, 2024 16:09

rfgamaral force-pushed the macos-text-replacement branch from b3d9ba0 to 900461d Compare August 21, 2024 16:31

rfgamaral force-pushed the macos-text-replacement branch from 900461d to c237e23 Compare August 21, 2024 17:46

rfgamaral force-pushed the macos-text-replacement branch from c237e23 to 5d57bd6 Compare August 21, 2024 18:40

fix: Broken text replacement feature (macOS-only)

a551d7e

rfgamaral force-pushed the macos-text-replacement branch 2 times, most recently from 1502aeb to 5f76c75 Compare August 21, 2024 21:11

rfgamaral force-pushed the macos-text-replacement branch from 5f76c75 to f042222 Compare August 22, 2024 09:00

Fix broken Cypress tests

e918404

rfgamaral force-pushed the macos-text-replacement branch from f042222 to e918404 Compare August 22, 2024 09:24

nperez0111 reviewed Aug 22, 2024

View reviewed changes

nperez0111 approved these changes Aug 22, 2024

View reviewed changes

bdbch approved these changes Aug 22, 2024

View reviewed changes

nperez0111 merged commit 88e310b into ueberdosis:develop Aug 22, 2024
14 checks passed

rfgamaral deleted the macos-text-replacement branch August 22, 2024 10:51

nperez0111 mentioned this pull request Aug 27, 2024

Tiptap 2.7 #5560

Merged

5 tasks

Nantris mentioned this pull request Sep 7, 2024

[Bug]: 2.7.0-pre.0 breaks some input rules and breaks undoing many more of them #5598

Closed

1 task

nperez0111 added a commit that referenced this pull request Sep 16, 2024

revert: "fix(core): handle macOS text expansion properly in InputRules (

e0c4db9

#5261)" This reverts commit 88e310b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Broken text replacement feature (macOS-only) #5261

fix: Broken text replacement feature (macOS-only) #5261

rfgamaral commented Jun 21, 2024

netlify bot commented Jun 21, 2024 •

edited

Loading

rfgamaral commented Jun 21, 2024

nperez0111 commented Jun 25, 2024

rfgamaral commented Jul 29, 2024

changeset-bot bot commented Aug 13, 2024 •

edited

Loading

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

rfgamaral commented Aug 22, 2024 •

edited

Loading

nperez0111 Aug 22, 2024

nperez0111 Aug 22, 2024

rfgamaral Aug 22, 2024

nperez0111 Aug 22, 2024

nperez0111 left a comment

bdbch left a comment

Nantris commented Sep 7, 2024

nperez0111 commented Sep 17, 2024

Nantris commented Sep 17, 2024

fix: Broken text replacement feature (macOS-only) #5261

fix: Broken text replacement feature (macOS-only) #5261

Conversation

rfgamaral commented Jun 21, 2024

Changes Overview

Implementation Approach

Testing Done

Verification Steps

Additional Notes

Checklist

netlify bot commented Jun 21, 2024 • edited Loading

✅ Deploy Preview for tiptap-embed ready!

rfgamaral commented Jun 21, 2024

nperez0111 commented Jun 25, 2024

rfgamaral commented Jul 29, 2024

changeset-bot bot commented Aug 13, 2024 • edited Loading

⚠️ No Changeset found

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

nperez0111 commented Aug 21, 2024

rfgamaral commented Aug 21, 2024

rfgamaral commented Aug 22, 2024 • edited Loading

nperez0111 Aug 22, 2024

Choose a reason for hiding this comment

nperez0111 Aug 22, 2024

Choose a reason for hiding this comment

rfgamaral Aug 22, 2024

Choose a reason for hiding this comment

nperez0111 Aug 22, 2024

Choose a reason for hiding this comment

nperez0111 left a comment

Choose a reason for hiding this comment

bdbch left a comment

Choose a reason for hiding this comment

Nantris commented Sep 7, 2024

nperez0111 commented Sep 17, 2024

Nantris commented Sep 17, 2024

netlify bot commented Jun 21, 2024 •

edited

Loading

changeset-bot bot commented Aug 13, 2024 •

edited

Loading

rfgamaral commented Aug 22, 2024 •

edited

Loading