cjohansen / replicant

A native ClojureScript virtual DOM renderer - render hiccup directly
222 stars 10 forks source link

Error with emptied out contenteditable content #15

Closed larstvei closed 10 months ago

larstvei commented 10 months ago

Hi,

It is lovely to see a pure Clojure rendering library!

I get an exception when I have an unholy mix of contenteditable div, that is cleared out, and then attempted to be reconciled with an added div (sorry, it's a bit hard to explain, but I hope the example below will clarify it).

Running this (contrived) example should reproduce the issue:

(ns replicant.contenteditablebug
  (:require [replicant.dom :as d]))

(defonce store (atom (list "Deleting the contents of this div, then clicking add, reproduces the bug.")))

(defn app [fields]
  [:div
   [:h1 "A bug that occurs when content is editable"]
   [:button {:on {:click #(swap! store conj "another field")}} "add"]
   (for [field fields] [:div {:contenteditable true :style {:border "solid red"}} field])])

(defn start []
  (set! (.-innerHTML js/document.body) "<div id=\"app\"></div>")
  (def el (js/document.getElementById "app"))
  (add-watch store :re-render (fn [_ _ _ _] (d/render el (app @store))))
  (swap! store identity))

(defonce _ (start))

I'm not entirely sure what is causing the issue, but it seems to occur when trying to reconcile some divs.

The program doesn't error if I comment out this:

https://github.com/cjohansen/replicant/blob/5c42514235717b93706d4b523a22384db808e759/src/replicant/core.cljc#L629-L633

Not a proposed solution, but an observation that might give you a clue as to what is going on.

cjohansen commented 10 months ago

Thanks for giving it a shot!

This is an interesting case - I hadn't even thought about it ๐Ÿ˜… Replicant assumes the DOM is only modified by itself. In this case its internal representation of the DOM says that the div has a text node child, so the next render should be able to reconcile the DOM quickly by replacing the old text node with the new one. Unfortunately, the child Replicant wants to replace no longer exists, since the end user was allowed to remove it, thanks to contenteditable...

I'll have to stew a little on what the best solution for this is. contenteditable is a lot like innerHTML in that it sends a signal that the DOM under this node will be mangled outside of Replicant. I think the best approach is for Replicant to always recreate the children.

cjohansen commented 10 months ago

I gave it a shot, but now I'm even more confused than when I started ๐Ÿ˜… How would you expect this to work? If you start by rendering this:

[:div {:contenteditable true} "Hello"]

And then later render this:

[:div {:contenteditable true} "Goodbye"]
  1. What should Replicant do?
  2. If I go in the browser and "contentedit" the "Hello" text to something else before the second render - what should Replicant do?

If you remove :contenteditable, Replicant should remove the existing text node ("Hello") and replace it with the new one (" Goodbye"). But if a user has edited the contents of "Hello", should Replicant still replace it? Or add "Goodbye"?

larstvei commented 10 months ago

Sorry for the confusion ๐Ÿ˜… The example is really contrived, and I wouldn't expect it to work in the sense that it does anything sensible. For it to make sense, you would probably have to synchronize the store and the content of the div.

That replicant assumes that DOM content is immutable is kind of what I expected.

I wouldn't expect that Replicant respects the state introduced by contenteditable state (though it shouldn't crash). I think the basic premise of Replicant is that what is rendered is a function of the state given as input. The state of the div is not given as input, and then I don't think Replicant needs to respect it either.

cjohansen commented 10 months ago

Got it working. Thanks for providing a reproduction ๐Ÿ‘

larstvei commented 10 months ago

Great!