search

Matt-Esch / virtual-dom

A Virtual DOM and diffing algorithm
MIT License
11.65k stars 780 forks source link

Percent size is not supported for elements attribute width/height #412

Open berdario opened 7 years ago

berdario commented 7 years ago

virtualDom.create(virtualDom.h('img', {width: '100.0%'})).width

Will yield 0

bendrucker commented 7 years ago

That is not an acceptable DOM value. It's being dropped by the DOM not virtual-dom. Only a number (of pixels) is valid.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-width

berdario commented 7 years ago

Is it? If I use the developer tools I can set 100% without any problem, in both Firefox and Chrome

berdario commented 7 years ago

it'd be ok to not accept it, but log when that's happening, rather than failing silently

berdario commented 7 years ago

Btw, I confirm that adding manually an element with document.body.appendChild(()=>document.createElement()...etc.etc.) yields an element with width 0

Weird how instead setting it in the devtools is possible...

bendrucker commented 7 years ago

it'd be ok to not accept it, but log when that's happening, rather than failing silently

No, it's not virtual-dom's job to reimplement huge chunks of complicated DOM APIs like that. The DOM's behavior is to drop invalid values. Better to be aware of that than avoid it.

jgaskins commented 7 years ago

@berdario When you set it in developer tools, you're setting element.style.width rather than element.width. The solution is to use h('omg', { style: { width: '100%' } }). The style property uses the browser's layout engine, so use all the CSS units.

it'd be ok to not accept it, but log when that's happening, rather than failing silently

Unfortunately, as @bendrucker points out, the JS DOM API doesn't do anything when you set properties to invalid values or even when you try to write to a read-only property, in order for virtual-dom to warn or do anything in response, it would have to explicitly verify every property you set. On a large render, this would probably be at least a 2x slowdown — potentially a lot more after you cover all the bases.

It's more feasible for devs to know the DOM API, unfortunately.

berdario commented 7 years ago

@jgaskins

Yup, I forgot to update you here, but I fixed this downstream (since I was an user of virtual-dom indirectly via purescript-halogen):

https://github.com/slamdata/purescript-halogen/pull/321

This has the advantage of preventing the errors for other developers unaware of the nuances between Element's width and CSS's width, while at the same time not having any impact/slowdown at runtime, since all of the checks are done at compile-time :)

I guess you can close this issue