[lexical][lexical-playground] Bug Fix: Add workarounds for buggy browser behavior around macOS text replacements#8417
Conversation
…s in text replacements
…macOS text replacements
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This might not actually be possible due to Chrome's bad heuristics. Still looking into it myself. EDIT: I think I figured it out now. It's a bit wonky but it does seem to work. |
etrepum
left a comment
There was a problem hiding this comment.
I think we should try and smooth out all of the wonky text replacement behavior we can find in one pass, the behavior in chrome with this one is still a bit wonky. For example pressing return or space should accept the replacement then do the behavior of that key. Right now it just accepts the replacement and effectively prevents default (haven't done a thorough audit of that section of the events code to step through what it's doing, just QA). The best justification to do it all at once is to make sure that we add the minimum amount of implicit state machine to make it work rather than add a little bit for each workaround.
Safari seems to have the right behavior before this patch, can also be tested in chrome in any textarea.
| handledSelectionCommandTimeoutId = setTimeout(() => { | ||
| isInsertTextAfterHandledSelectionCommand = false; | ||
| handledSelectionCommandTimeoutId = null; | ||
| }, 0); |
There was a problem hiding this comment.
Looks like this is effectively the same as the above function, should save a few bytes to just call it
| handledSelectionCommandTimeoutId = setTimeout(() => { | |
| isInsertTextAfterHandledSelectionCommand = false; | |
| handledSelectionCommandTimeoutId = null; | |
| }, 0); | |
| handledSelectionCommandTimeoutId = setTimeout(clearHandledSelectionCommandInsertText, 0); |
| } else if (isSelectAll(event)) { | ||
| event.preventDefault(); | ||
| dispatchCommand(editor, SELECT_ALL_COMMAND, event); | ||
| if (dispatchCommand(editor, SELECT_ALL_COMMAND, event)) { | ||
| markHandledSelectionCommandInsertText(); | ||
| } |
There was a problem hiding this comment.
As far as I can tell this can be lifted up after isRedo and not repeated
|
@etrepum yeah, I'm not particularly happy with fix. This patch of Chromium I submitted for Electron fixes it for Chrome: electron/electron#50432 To note, Chrome is the only browser between it, Firefox, and Safari, that has this bug. #8414 is a different story. Firefox and Chrome exhibit that bug, but Safari doesn't. Both bugs closely related, and there's spec questions to be had at how all three browsers implement |
I find
to be two separate bugs (which is why I filed them separately). Bug 1 only exists in Chrome and is arguably because it has the worst implementation of Again, we can't really change any of this, only work around it. |
|
I agree that conceptually these are different things because there are different keystrokes involved and have slightly different browser compatibility matrices. However, as far as reviewing goes it would be much easier and more efficient for me if I had one PR to review that worked around all of the known bad behavior with macOS text replacements. This PR for example is shipping without any sort of test infrastructure (playwright doesn't really make it feasible to write real tests for IMEs anyway) and requires manual QA in all of the browsers, so it would be way better if I only had to go through that process for one PR. |
Understandable, I can combine everything into this PR if you prefer, since we're having the convo here already. |
|
What I would like to see working (with the selection on the rightmost character except in the arrow key cases):
Ideally this would just be one PR where I can carefully read through the code and all of the required context and run through all of the scenarios in each browser to verify expected behavior. With multiple PRs I have to repeat all of this. In the current playground all of these work as expected in Safari, but the behavior is mixed in the other browsers |
|
Rolling it up into this PR works for me |
…ext-replacements-accepted-by-backspace
|
A long bit of context for these bugs. The bugsThe two bugs:
The
|
etrepum
left a comment
There was a problem hiding this comment.
This is much improved! I've found two edge cases that we should consider handling before this is merged:
Chrome - if you complete the text replacement with the same character that the replacement ends with, the selection ends up before the last character you typed.
Screen.Recording.2026-04-30.at.07.48.07.mov
Firefox - punctuation doesn't accept the text replacement
Screen.Recording.2026-04-30.at.07.48.57.mov
This appears to be the current behavior before my patches in both I will investigate the Chrome bug, though. |
|
The lingering Chrome bug has been squashed! I left the Firefox bug as-is because it appears to be consistent across all kinds of inputs in that browser. |
etrepum
left a comment
There was a problem hiding this comment.
QA looks great, agreed that it's fine to leave the Firefox behavior as-is. Will merge after taking a closer look at the code when I have the time.
Description
This adds workaround behavior where typing a character to accept a macOS text replacement used to leave the caret before that character instead of after. It also adds workaround behavior where pressing Backspace (and performing Select All, apparently) would accept a pending macOS text replacement instead of deleting a character.
Closes #8413
Closes #8416
Test plan
There are tests added for the caret placement bug. The Backspace / Select All acceptance bug relies on native behavior that can't easily be synthesized, so there are no tests around that.
Before
Screen.Recording.2026-04-28.at.14.11.05.mov
Screen.Recording.2026-04-28.at.16.02.05.mov
After
Screen.Recording.2026-04-28.at.14.11.32.mov
Screen.Recording.2026-04-28.at.16.02.35.mov