Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composition text replacement in Safari removes empty nodes during text composition text deletion #934

Open
jljorgenson18 opened this issue Jun 6, 2019 · 14 comments

Comments

@jljorgenson18
Copy link

jljorgenson18 commented Jun 6, 2019

Issue details

This is a Safari issue that we've dealt with in the past and it looks like this is happening in Prosemirror as well. Let's say you have a structure that looks like this

<ul>
  <li>
    <div></div>
  </li>
</ul>

and the cursor is inside of the "div" tag. If you start typing with a composition based IME (like Japanese) and then attempt to finish a composition, Safari fires an input event with an inputType of deleteCompositionText. This deletes the text before it inserts the final composition text. Now, what Safari does during the delete step is that any nodes that ended up as "empty" that were wrapped around the text that was just replaced are removed. In the example above, this means that if you started typing in the div tag, then the div would be removed, then the li, then the ul. Here it is in action

ezgif-4-09fd8029caa1

The workaround we currently have in place is trick Safari into thinking that the element is not empty. We have a plugin that looks like this.

        props: {
            handleDOMEvents: {
                beforeinput: (view: EditorView, event: InputEvent): boolean => {
                    if (event.inputType !== 'deleteCompositionText') {
                        return false;
                    }
                    const selection = window.getSelection();
                    const range = selection.getRangeAt(0);
                    const { startContainer, endContainer, endOffset, startOffset } = range;
                    /**
                     * On Safari when composition text is deleted, it deletes any empty elements it finds up the dom tree. Prosemirror can sometimes be confused by this
                     * and will think that we meant to delete those elements. This fix forces the resulting node to not be empty.
                     * The condition here checks to see if the entire text node is about to be swapped inside of an element
                     */
                    if (
                        isTextNode(startContainer) &&
                        startContainer === endContainer &&
                        startOffset === 0 &&
                        endOffset === (startContainer as Text).length
                    ) {
                        startContainer.parentElement.insertBefore(document.createTextNode(ZWS), startContainer);
                    }
                    return false;
                },
                input: (view: EditorView, event: InputEvent): boolean => {
                    if (event.inputType !== 'deleteCompositionText') {
                        return false;
                    }
                    const selection = window.getSelection();
                    const range = selection.getRangeAt(0);
                    removeZWS(range.startContainer);
                    return false;
                }
            }
        }

I would expect the domObserver not to honor the element removal, but I'm wondering if the recent composition changes broke this behavior as I'm not seeing it occur on the example editor.

@marijnh
Copy link
Member

marijnh commented Jun 7, 2019

I would expect the domObserver not to honor the element removal, but I'm wondering if the recent composition changes broke this behavior as I'm not seeing it occur on the example editor.

I can't quite parse this. What do you mean by honoring the element removal?

The website example editor should be using the latest version. What are you not seeing in it?

@jljorgenson18
Copy link
Author

Sorry for the confusion, I had misunderstood something on the example editor and thought it was pointing to an older version.

What I mean by "honoring the element removal" is that when the empty elements are removed, the model itself shouldn't be updated and the dom should revert to the expected structure. So even though the ul, li, and div get deleted, they are each restored when the mutation observer picks up the changes.

I can try to track down the actual filed bug in Safari, but my guess as to why I don't see it in the example editor is due to the "div". It might trigger this element removal if the text has a div wrapped around it. We saw this behavior in our old editor in both lists (where we have a div inside of the li) and in tables (where we had a div inside of a td). I think if the toDOM method of the list schema included a div inside of the list item, then it would happen in the example editor. Do you have any suggestions on creating a reproducible example?

@marijnh
Copy link
Member

marijnh commented Jun 7, 2019

Do you have any suggestions on creating a reproducible example?

You could adjust the basic schema to render paragraphs as divs.

It is well possible that the changes in 1.9.0 made this harder to work around (they make the DOM reading more eager).

@jljorgenson18
Copy link
Author

So after a lot of struggle, it turns out that this happens when the list and list item are both position relative. I created a working example here https://glitch.com/edit/#!/geode-shame?path=index.js:241:16. No idea why this is the case, but that plugin fix still seems to do the trick.

@Pxygogogo
Copy link

I just had the same problem.
I'm wondering if there is any new progress or other solutions? thanks

@vivaxy
Copy link

vivaxy commented Mar 30, 2022

Example to reproduce the issue: https://peaceful-luminous-provelone.glitch.me.
Mar-30-2022 21-21-05

Just added position: relative to nodes. I think it's related with the position: relative.

@jljorgenson18
Copy link
Author

jljorgenson18 commented Mar 30, 2022

For the record, we improved upon the original solution and instead now use a decoration with an empty span instead of a ZWS. We basically just detect the bad browser state in beforeinput, update our input plugin state to toggle a flag for composition text being deleted, render the empty span decoration on Safari when that flag is true, then unset in the input event. Using state and decorations helped to better trace what is happening and allowed us to better guarantee that the hack element gets removed.

Edit: Forgot that we actually put a ZWS inside of the span in our decoration.

@Pxygogogo
Copy link

Thanks for the reply, decoration is indeed a better implementation and has no effect on the data.
By the way, have you guys researched the reason for this behavior in Safari? We found what appears to be an issue with Safari's MutationObserver, but haven't looked into it further.

@jljorgenson18
Copy link
Author

jljorgenson18 commented Mar 30, 2022

Safari, in general, is a disaster with contenteditable. Last I checked, this happens in a plain contenteditable element without Prosemirror. The root of the issue is that the webkit contenteditable code is bad 🤷🏻

@Pxygogogo
Copy link

Pxygogogo commented Mar 31, 2022

this happens in a plain contenteditable element without Prosemirror.

It was true, and it is still the same(at least in Safari 14.1). Thank you for your patience, to be honest, Safari really surprised me. There's still a lot to learn 🆙

@percy507
Copy link

+1. Same problem on safari browser 15.0.

2022-04-24 08 38 08

@percy507
Copy link

percy507 commented Jul 2, 2022

+1. Same problem on safari browser 15.0.

For my case, remove the css(position:relative;) of the heading elements can resolve my issue. I can't figure out why.

I also made a test in the official example. Add css position:relative for heading elements through safari's devtool, the same issue appears. 😂

@marijnh Do you know why?

@jljorgenson18
Copy link
Author

@percy507 Safari is trash at contenteditable. That is why. It isn't a Prosemirror specific issue.

@ocavue
Copy link

ocavue commented Feb 25, 2023

I also see this issue when developing prosemirror-flat-list, so I created a plugin as a workaround for this Safari bug, based on @jljorgenson18's idea.

import { createSafariInputMethodWorkaroundPlugin } from "prosemirror-flat-list"

const plugin = createSafariInputMethodWorkaroundPlugin()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants