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

Updating a contenteditable w/ on:input function is prepending, not replacing, if initial value is empty #5018

Closed
noahlh opened this issue Jun 15, 2020 · 11 comments

Comments

@noahlh
Copy link

noahlh commented Jun 15, 2020

Describe the bug
Sorry for the mouthful of a title. Showing is often easier than telling:

https://svelte.dev/repl/04d08576e3db43d6a7f5c3fd7ae593a3?version=3.23.2

If the initial value is empty, typing into the contenteditable div causes the value to be prepended with the previous value after each keystroke (instead of replaced). Eg typing "Hello" nets a value of:

HelloHelloHellHelHeH

This does not happen if a) the initial value of the field is non-empty or b) after you complete that initial input, you select all the garbage text, delete it, and re-enter new text.

Expected behavior
The value of the element's innerText should reflect exactly what is typed.

Information about your Svelte project:

  • Chrome 81 & Safari 13.1.1
  • MacOS 10.15.5
  • Svelte 3.23.2

Severity
Annoying / semi-blocking (I can work around this in my project, but it does prevent me from initializing the div with an empty value, which I'd prefer)

Additional context
Checked with #support on Discord and was suggested that I file a bug. I searched and couldn't find any related issues. Apologies in advance if this is my mistake and not actually a bug!

@skippednote
Copy link
Contributor

@noahlh You can use binding bind:textContent={text} instead of placing the content within the element.
https://svelte.dev/repl/a3c3b7e51b2647f3b8aebef533e93321?version=3.23.2

However, this does seem to be a bug to me. I'm looking into it.

@skippednote
Copy link
Contributor

I'm getting positive results if we check for the wholeText property on the childNode instead of data.

Swapping the following

export function set_data(text, data) {
	data = '' + data;
	if (text.data !== data) text.data = data;
}

with the this

export function set_data(text, data) {
	data = '' + data;
	if (text.wholeText !== data) text.data = data;
}

https://github.com/sveltejs/svelte/blob/master/src/runtime/internal/dom.ts#L196

@noahlh
Copy link
Author

noahlh commented Jun 16, 2020

@skippednote Awesome!! Thanks for the quick ID & fix on this.

I did originally use the 2-way binding (which indeed works perfectly), but unfortunately for my application I need to do manual binding, since contenteditable gets set dynamically based on component state, and Svelte doesn't support 2-way binding w/ a dynamic attribute on contenteditable.

@Conduitry
Copy link
Member

@noahlh
Copy link
Author

noahlh commented Jul 10, 2020

@Conduitry @skippednote Sorry to be difficult, but I think this might not quite be fixed -- check out the REPL link above, but start typing and hit "enter" in the field. Same issue reappears :(

@skippednote
Copy link
Contributor

@noahlh The URL you had shared earlier uses an older version of Svelte

Try this https://svelte.dev/repl/04d08576e3db43d6a7f5c3fd7ae593a3?version=3.24.0

@noahlh
Copy link
Author

noahlh commented Jul 10, 2020

@skippednote Sorry - i wasn't clear. I was indeed referring to the link that Conduitry shared (same as yours). The issue appears resolved IF you only type text. But try typing some text then hitting Enter (eg putting in a line break).

@RaiVaibhav
Copy link
Contributor

Hey @noahlh
The reason why content editable worked when clear the input is because mustache tag which is actually a Text node exits inside the editable div, so on clear it gets removed from the editable div, after that there wasn't any text node (which is binded with the state value) and why replication of value happening on pressing next line or enter because {text} (or say Text node) already exits inside the editable div and when we enter something like a\na\na we set this directly to the data of Text node but instead it should be innerText. (this part need the fix in svelte code)

I will definitely say no to the using contenteditable case with a mustache as a child element, it will always create confusion everywhere for cases like <div contenteditable=true on:input={handleInput}>{text}<b>some</b></div> and begineers who is dealing with will have a lot of confusion till then they go through the mdn docs, I think svelte needs to add a warning or info to avoid these cases.

I will open the PR soon for review

RaiVaibhav added a commit to RaiVaibhav/svelte that referenced this issue Oct 18, 2021
Fixes: sveltejs#5018
Fixes: sveltejs#5931

Issue caused because of setting the updated value to
the data property of moustache text node, use innerText if
parent have contenteditable attribute
@RaiVaibhav
Copy link
Contributor

After digging a little bit, realised innerText will not even work in the first place, a wrong approach, mostly IMO there should be a warning from svelte to avoid this kind of regression and use binding only.

@johnnysprinkles
Copy link

If this has to do with two adjacent text nodes can we just make an action that constantly normalizes? Seems to work here:

https://svelte.dev/repl/6a2b5691f4c04889b9a68b3f60be6ce3?version=3.23.2

dummdidumm added a commit that referenced this issue Mar 21, 2023
- split logic up into "is this a contenteditable element" and depending on the outcome use either .wholeText or .data to check if an update is necessary
- add to puppeteer because jsdom does not support contenteditable
- one test is skipped it because it fails right now but helps test #5018

---------

Co-authored-by: suxin2017 <1107178482@qq.com>
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
@dummdidumm
Copy link
Member

Using contenteditable is a nasty beast, and there's not much Svelte can do to make this behave correctly in all situations. Back when this issue was created bind:innerText didn't exist yet, but it now does, and it should be used instead.

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

Successfully merging a pull request may close this issue.

7 participants