guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.5k stars 245 forks source link

Virtual DOM for formatters #236

Open OliverJAsh opened 10 years ago

OliverJAsh commented 10 years ago

Regarding issue https://github.com/guardian/scribe/issues/209, I've started to experiment with how we might use a virtual DOM for the formatters in Scribe.

Currently we reset Scribe’s innerHTML every time the formatters are run, which means that we lose all references to nodes inside the Scribe DOM. Some plugins need these references. This is also very bad for performance: innerHTML will cause the layout to be recalculated and re-painted. A virtual DOM would solve both of these problems because we would only ever mutate parts of the Scribe DOM which the formatters make changes to.

Here's what I've got so far: https://gist.github.com/OliverJAsh/cedbd215a8bdf162cf4b

This mimics the curly quotes formatter. I mutate the text nodes, diff my new tree against the current tree, and apply the diff as a patch.

DOM references are kept intact, even for text nodes. I was surprised by this – I thought it would just replace the text node if it's contents are different, but it just updates the textContent/data instead. See the console.

I need a way to clone the current tree (because I don't want to mutate the current tree). I wasn't sure how to do it. Any thoughts? See line 32.

This made me realise, for the formatters to run, we'll have to build a new virtual tree out of the Scribe DOM. We could traverse the actual DOM to re-create an identical virtual DOM?

/cc @theefer @robinedman @hmgibson23

OliverJAsh commented 10 years ago

It turns out there's a module to virtualise a real DOM. Updated the gist. https://gist.github.com/OliverJAsh/cedbd215a8bdf162cf4b

OliverJAsh commented 10 years ago

The gist now demonstrates how you could use a virtual DOM for the Scribe formatters. Unfortunately I haven't got much further with this, because the virtual-dom library is written in CommonJS which means we are unable to consume it (see https://github.com/jspm/jspm-cli/issues/75#issuecomment-50616067).

I can't see how we can import this library without moving to jspm – there might be another way.

Me and @theefer were also wondering how we could tidy up the code in the gist. See https://github.com/Matt-Esch/virtual-dom/issues/101.

OliverJAsh commented 10 years ago

@theefer @robinedman Just wanted to note that you could taking a look at using the new jspm bundle-sfx: https://github.com/systemjs/builder/issues/2#issuecomment-51415882

It's not exactly what we want but it might be good enough!

Raynos commented 10 years ago

@OliverJAsh

if you want an easier way of importing modules you can use mercury which has a dist/mercury.js file ( https://github.com/Raynos/mercury/blob/master/dist/mercury.js ) with pretty much everything you need.

mitar commented 10 years ago

What is the progress of this? We would need this functionality for exactly the same reason: rendering math equations inside Scribe. Is there a branch to try?

OliverJAsh commented 10 years ago

Unfortunately I'm no longer able to maintain Scribe, but AFAIK it's still in heavy use at The Guardian.

@sihil Do you know if any progress has been made on this issue?

sihil commented 10 years ago

Hi @mitar - it's not on our immediate roadmap, but we will get round to it at some point. We've been exploring using a virtual DOM inside one of our newer plugins and are keen to expand that to underpin scribe. Will most likely be a breaking change for all existing plugins in order to keep it simple.

mitar commented 10 years ago

Is there any roadmap on this? Is there any preliminary code on which we could work in parallel?

sihil commented 10 years ago

Aside from the spike that Oliver did there isn't any preliminary code. @robinedman might have some opinions on the best place to start.

robinedman commented 10 years ago

Hi there @mitar. The plugin @sihil mentioned is scribe-plugin-noting.

There's talks about possibly creating a "Scribe v2" that would be based on virtual-dom, maybe the full mercury framework since we might end up using a lot of it anyway. But as @sihil says, it's not on our immediate roadmap.

bregenspan commented 9 years ago

A strategy we're starting to try out at Gawker (not ready yet though) is only formatting nodes that just mutated. This should allow only running formatters on e.g. a paragraph that just changed, and make it possible for formatters to leave things like embedded content such as Iframes or elements with bindings untouched (or at least touched much less frequently). Will update if we find a solution that seems like it makes sense for merge upstream!

hmgibson23 commented 9 years ago

This sounds like a good approach. We've been talking about something similar but haven't actually got round to trying it yet. Let us know how it goes!

bregenspan commented 9 years ago

FWIW that approach got quite complicated (each formatter needed both the HTML or element it was modifying AND the surrounding context of the element, and to somehow enforce a pattern of only writing to the element it was operating on and never writing to its surrounding context). So we're actually trying out virtual DOM. Even the following pretty naive and wasteful approach works shockingly well (though not yet ready enough for me to make an upstream PR), and leads to all tests passing:

-            scribe.setHTML(scribe._htmlFormatterFactory.format(scribe.getHTML()));
+            var html = scribe.getHTML();
+            var formattedHtml = scribe._htmlFormatterFactory.format(html);
+            if (html !== formattedHtml) {
+              var vtree = virtualize.fromHTML('<div>' + html + '</div>');
+              var vtree2 = virtualize.fromHTML('<div>' + formattedHtml + '</div>');
+              scribe.el.normalize();  // merge any adjacent text nodes - needed for virtual DOM to apply patches correctly
+              patch(scribe.el, diff(vtree, vtree2));
hmgibson23 commented 9 years ago

:+1: for this. This looks awesome. I'm looking forward to the upstream PR.

theefer commented 9 years ago

The ultimate goal was to move all plugins to deal with virtual dom rather than real DOM objects, so that's a step in the right direction!

I don't believe running plugins on "diffs" (what mutated) would work as many plugins are highly contextual.

bregenspan commented 9 years ago

@theefer totally agreed -- my goal at the moment is to find a short-term solution that could allow for working with the existing formatters while solving the issue of IFRAMEs refreshing and element references being lost during formatting. I'm a little surprised by the cheapness of transforming HTML to virtual DOM and how well the diff approach works on its own, but it's likely not the ideal long-term solution, which would probably look a lot closer to Medium's implementation, just with some persistent virtual DOM used as the "model".

There are some cases where the HTML->virtual DOM method (which is just https://github.com/marcelklehr/vdom-virtualize ) could use some extra hints to be passed in in order to preserve existing elements in the diff, so looking into those as well as profiling performance.