diff and patching nodes with contentEditable='inherit' #176

RGBboy · 2015-01-22T04:31:05Z

If the contentEditable property is not specified on a node to update and the existing node has it, virtual-dom tries to set it to a blank string. This throws the following error in Chrome and Firefox:

Uncaught SyntaxError: Failed to set the 'contentEditable' property on 'HTMLElement': The value provided ('') is not one of 'true', 'false', 'plaintext-only', or 'inherit'.

Here is my setup that produces the error:

<!DOCTYPE html>
<html lang="en">
  <head>
    <script src="script.js"></script>
  </head>
  <body>
    <div id="App"></div>
  </body>
</html>

var h = require('virtual-dom/h'),
    diff = require('virtual-dom/diff'),
    patch = require('virtual-dom/patch'),
    createElement = require('virtual-dom/create-element'),
    virtualize = require('vdom-virtualize'),
    document = global.document,
    element = document.getElementById('App'),
    tree = virtualize(element),
    newTree = h('div'),
    patches = diff(tree, newTree);

element = patch(element, patches);

I also put together a requirebin to show the problem: http://requirebin.com/?gist=d96693e487580d462d4f

When vdom-virtualize creates the tree it finds the contentEditable property is set to inherit. Then when the new tree is created contentEditable is not specified so a patch is created to remove it. This then causes the error because contentEditable must be set to one of true, false, plaintext-only, or inherit. inherit is the default value.

There are a couple of solutions to this. Either when a node is created, if contentEditable is undefined it is set to inherit or when nodes are diffed if one node has contentEditable set to inherit and the other undefined then the diff is nothing.

The text was updated successfully, but these errors were encountered:

Raynos · 2015-01-22T04:47:13Z

Sounds like a virtualize bug.

RGBboy · 2015-01-22T05:56:24Z

This gist exhibits the same problem without virtualize: http://requirebin.com/?gist=d96693e487580d462d4f

var h = require('virtual-dom/h'),
    diff = require('virtual-dom/diff'),
    patch = require('virtual-dom/patch'),
    createElement = require('virtual-dom/create-element'),
    domify = require('domify'),
    html = '<div id="App">Before</div>',
    element = document.body.appendChild(domify(html)),
    oldTree = h('div', { id: 'App', contentEditable: 'inherit' }, 'Before'),
    newTree = h('div', { id: 'App' }, 'After'),
    patches = diff(oldTree, newTree);

element = patch(element, patches);

neonstalwart · 2015-01-22T19:28:55Z

@RGBboy does it work if you explicitly provide the new value for contentEditable? virtual-dom tends to prefer you be explicit rather than assume what it should do when a property is missing between 2 trees.

RGBboy · 2015-01-22T21:20:25Z

@neonstalwart it works if you do something like the following:

var h = require('virtual-dom/h'),
    diff = require('virtual-dom/diff'),
    patch = require('virtual-dom/patch'),
    createElement = require('virtual-dom/create-element'),
    domify = require('domify'),
    html = '<div id="App">Before</div>',
    element = document.body.appendChild(domify(html)),
    oldTree = h('div', { id: 'App', contentEditable: 'inherit' }, 'Before'),
    newTree = h('div', { id: 'App', contentEditable: true }, 'After'),
    patches = diff(oldTree, newTree);

element = patch(element, patches);

The issue is that the following two nodes will map to a dom element with contentEditable set to inherit:

var nodeA = h('div');
var nodeB = h('div', { contentEditable: 'inherit' });

Anyone trying to convert existing dom elements back to vnodes will get a contentEditable: 'inherit' property unless you explicitly ignore contentEditable on elements with contentEditable: 'inherit'.

Raynos · 2015-02-14T20:15:43Z

This is a bug in virtual-dom.

Virtual-dom will set a property to '' if it finds undefined. The DOM does not allow setting this field to ''

@Matt-Esch we need to either:

do a safe set (in a try catch)
have a white list of known properties that throw and do a safe set
have a white list of known properties that throw and have a default value different from ''

matthewp · 2015-02-14T22:33:17Z

Wouldn't the try/catch option mean it would happen on every redraw? If so I think one of the whitelist options is a better way to go.

Zolmeister · 2015-05-06T17:47:51Z

+1
workaround: https://github.com/Zorium/zorium/blob/master/src/render.coffee#L31

alexmingoia · 2015-05-06T23:08:16Z

Why can't we just call removeAttribute? Why set attribute that are specified as being removed by a patch operation as ""?

bitinn · 2015-05-07T05:17:57Z

I have tried using html-to-vdom with my PE approach and it works without throwing error mentioned. So htmlparser2 definitely do not add contentEditable: inherit on dom like browser's dom parser.

There can be 2 solutions to this:

Try to get vdom-virtualize use a special case to ignore browser default behaviour.
Or as mentioned by Raynos, virtual-dom protects user from making this mistake.

bitinn · 2015-05-07T05:34:19Z

I have a pull request ready for vdom-virtualize, it works now, looks like contentEditable is the only edge case at the moment. marcelklehr/vdom-virtualize#22

You can try it using bitinn/vdom-virtualize#special-case

alexmingoia · 2015-05-07T05:46:42Z

@bitinn I do not think that is the proper way to solve this because the bug occurs without using vdom-virtualize as demonstrated by the previously posted requirebin example. node here in apply-properties.js is a DOM node, yet virtual-dom does not actually detect if the DOM node property being modified is legal to modify or not. It should only be modifying attributes (not all properties) and only in a manner valid according to the browser's DOM.

I think the proper solution is that virtual-dom must keep a whitelist of which DOM node properties are attributes and which aren't, and unfortunately, which modifications are legal in some cases.

alexmingoia · 2015-05-07T05:52:34Z

Sure it is not the simplest solution, but I think it is the proper one. We can't escape not dealing with the DOM's intricacies when working with it - as ugly as it is. The try/catch approach is not proper either because it just sweeps these issues under the rug and dings performance.

This is the same approach React uses for the same reasons. See this list: https://github.com/facebook/react/blob/0.13-stable/src/browser/ui/dom/HTMLDOMPropertyConfig.js

bitinn · 2015-05-07T06:11:06Z

@alexmingoia yeah I would also prefer a fix from virtual-dom end, but I just want to see what else is broken and try to get my PE approach working :)

egasimus · 2015-06-21T11:11:44Z

+1 for this issue. What shall we do about it?

bitinn · 2015-06-21T11:36:43Z

@egasimus Both vdom-virtualize and vdom-parser should have workaround this problem, how did you run into it?

(edited: sorry for the garbled message, office 365 doesn't work too well with github email reply it seems.)

egasimus · 2015-07-03T10:33:52Z

@bitinn I'm not using either of those (at least not directly -- I don't know if they're actually the underlying components of the workflow centered around h, diff, patch and create-element). I just tried toggling contenteditable in such a template and was faced with this error.

josdejong · 2016-08-03T18:36:11Z

Andy news on this bug? I encountered it too:

Uncaught SyntaxError: Failed to set the 'contentEditable' property on 'HTMLElement': The 
value provided ('null') is not one of 'true', 'false', 'plaintext-only', or 'inherit'.

josdejong · 2016-08-05T09:28:44Z

For others encountering the same issue, I found a workaround. What I did is change a DOM structure like this:

<div>
  <div contentEditable="true">B</div>
</div>

to something like this:

<div>
  <div>A</div>
  <div contentEditable="true">B</div>
</div>

Which gives trouble since in some cases virtual-dom tries to set contentEditable of the div A to "null".

To work around this, you have to make sure all siblings of the div's having a contentEditable attribute always have contentEditable specified too, like:

<div>
  <div contentEditable="false">A</div>
  <div contentEditable="true">B</div>
</div>

Raynos mentioned this issue Feb 14, 2015

Exception when initializing an app on an element with the same tag name Raynos/mercury#136

Closed

bitinn mentioned this issue May 6, 2015

Inquries: i18n interpolation, first-page problem #227

Closed

bitinn mentioned this issue May 7, 2015

special case for contentEditable marcelklehr/vdom-virtualize#22

Merged

marcelklehr mentioned this issue Jun 16, 2015

Remove special case for contentEditable marcelklehr/vdom-virtualize#30

Open

ghost mentioned this issue Aug 4, 2015

ContentEditable edge case - inherit dfilatov/vidom#87

Closed

thSoft mentioned this issue Nov 4, 2015

Failed to set the 'contentEditable' property #329

Closed

thSoft added a commit to thSoft/virtual-dom-1 that referenced this issue Nov 4, 2015

Fix Matt-Esch#176

8d96bde

josdejong mentioned this issue Aug 5, 2016

Issue when inserting a div before an editable div (contentEditable) preactjs/preact#258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff and patching nodes with contentEditable='inherit' #176

diff and patching nodes with contentEditable='inherit' #176

RGBboy commented Jan 22, 2015

Raynos commented Jan 22, 2015

RGBboy commented Jan 22, 2015

neonstalwart commented Jan 22, 2015

RGBboy commented Jan 22, 2015

Raynos commented Feb 14, 2015

matthewp commented Feb 14, 2015

Zolmeister commented May 6, 2015

alexmingoia commented May 6, 2015

bitinn commented May 7, 2015

bitinn commented May 7, 2015

alexmingoia commented May 7, 2015

alexmingoia commented May 7, 2015

bitinn commented May 7, 2015

egasimus commented Jun 21, 2015

bitinn commented Jun 21, 2015

egasimus commented Jul 3, 2015

josdejong commented Aug 3, 2016

josdejong commented Aug 5, 2016

diff and patching nodes with contentEditable='inherit' #176

diff and patching nodes with contentEditable='inherit' #176

Comments

RGBboy commented Jan 22, 2015

Raynos commented Jan 22, 2015

RGBboy commented Jan 22, 2015

neonstalwart commented Jan 22, 2015

RGBboy commented Jan 22, 2015

Raynos commented Feb 14, 2015

matthewp commented Feb 14, 2015

Zolmeister commented May 6, 2015

alexmingoia commented May 6, 2015

bitinn commented May 7, 2015

bitinn commented May 7, 2015

alexmingoia commented May 7, 2015

alexmingoia commented May 7, 2015

bitinn commented May 7, 2015

egasimus commented Jun 21, 2015

bitinn commented Jun 21, 2015

egasimus commented Jul 3, 2015

josdejong commented Aug 3, 2016

josdejong commented Aug 5, 2016