Preact: simple property assignment fails to set img width

[enapupe]

I'm facing some incompatibility when replacing react with preact.
It seems the way preact sets attributes (https://github.com/developit/preact/blob/master/src/dom/index.js#L68 - simply by setting a property) is not safe.

My use case is a simple img component that receives width and breaks with the value of 100% because of the the following:

const img = document.createElement('img');
img.width = '100%';
console.log(img.width); // 0

And what I think should be done (this is what react does)

const img = document.createElement('img');
img.setAttribute('width', '100%');
console.log(img.width); // expected value (automagically defined by the browser)

Unfortunately, changing setProperty to

function setProperty(node, name, value) {
  try {
    node.setAttribute(name, value);
  } catch (e) { }
}

isn't enough.. I think we might have to do some checks before knowing which kind of property setting we should do (simple assignment or setAttribute).. Any thoughts?

[developit]

Interesting issue for sure. Preact's size basically means whitelists are impossible, so it seems like we'll need a way to determine this programmatically, or a full move (back) to attributes for setters. Maybe String values could trigger attribute setting? (just pondering)

[robertknight]

For reference, the relevant part of the React code is https://github.com/facebook/react/blob/master/src/renderers/dom/shared/DOMPropertyOperations.js#L133 which references the attribute -> config bitflags defined in https://github.com/facebook/react/blob/master/src/renderers/dom/shared/HTMLDOMPropertyConfig.js

Looking at that second file, I'll note that the number of attributes which are marked as being required to be set as a property rather than an attribute is pretty small - only checked, muted, multiple and selected.

[leeoniya]

@developit your best bet is probably sticking with attribute setters as the default and have a short whitelist of very common ones that can be set by prop for perf. "class" and "id" bascially cover most of the perf-critical stuff.

also as @robertknight points out, the stuff that should be set by property assignment is also pretty small.

[robertknight]

Maybe String values could trigger attribute setting? (just pondering)

This idea seems worth exploring. It seems conceptually nicer to use property setters where possible and if Preact were to use setAttribute for boolean properties, you'd need some additional logic to translate the JS bool value to a string.

[developit]

preact used to use setAttribute() that way. Ends up getting a little iffy when you have to deal with complex objects like data, but maybe that's the easier thing to whitelist. This is tricky.

[enapupe]

Do you guys think a specific test case for width is worth it?

[developit]

@enapupe it's a little specific, but at least that lets us track the error. Much appreciated :)

[developit]

@robertknight Been pondering String -> setAttribute in the back of my head and I can't seem to come up with any issues. It could also almost handle class out-of-the-box!

[thysultan]

if name === width | height and typeof value === string use setAttribute, or you could use style.width and style.height. or setAttribute but whitelist className.

[thysultan]

@enapupe though both img.setAttribute('width', '100%'); and img.width = '100%' don't affect the width of the image, if you wanted to set the width to 100% you'd need style.width. If this works in react then react might be whitelisting string width values to use style.width

[enapupe]

@thysultan there's no need to check if width value is String, always setting it via setAttribute should just work.. Setting via style is bad idea because it might cause more incompatibility.

@developit I was thinking maybe we could import some tests from react? We don't need to commit them to preact, just use the logic to validate your idea of always use setAttribute if value is String.

[developit]

@enapupe I'm open to the test idea - it was also proposed in #242.

Regarding the String suggestion - the reason this is useful is because Preact cannot support property whitelists given the filesize constraints we have placed on the library. Instead, we need to look for other ways to implement conditions - if the String type of a value can be used to fix this bug without causing a performance degradation for other String props, that's probably what will end up being used since it's only a couple of bytes (and thus much smaller than a whitelist).

It's also worth noting that I think this issue would apply more broadly to other DOM properties. height is the obvious one, but there are others - for example list and type are already blacklisted for similar reasons. Maybe our solution to this issue will enable us to remove those specific blacklists and reduce the bundle size :)

[sergeychernyshev]

I was flabbergasted to hit this issue when I used Preact.

My JSX has simple <img src="..." width="100%"/> in it and it silently broke.

I replaced it with <img src="..." style={{width: "100%"}} /> to solve the issue, but it seems to be breaking obvious promise of web framework made by the library.

[developit]

It's an open bug, typically open bugs represent broken promises.

[sergeychernyshev]

I do realize that, just wanted to add an example of the solution for people who stumble on this - had hard time figuring it out from first pass reading this.

Seems like a big open issue that wasn't fixed since last comment on Jan 11.

Anyway, I do realize the open source challenge - maybe overreacted a bit ;)

[developit]

Makes sense. For some context, the reason I haven't jumped on this change yet is that the fix is generalized and could have adverse effects on other properties. I'm very hopeful and it won't, but it's a tough decision.

[enapupe]

@developit Maybe we could move forward in two fronts:

• fix the specific img issue (even if that means adding some undesired bytes)
• keep working on that complex generalized setAttribute situtation

[developit]

My fear there would be that we'd have two breaking releases back-to-back to fix the same issue. I have some time to dedicate to this and the issue is high-priority, so I think we can move towards the generalized solution first. It has interplay with a bunch of different high priority issues, like the WC interop stuff.

FWIW I think it would be super useful to have an automated test of this - maybe a giant map of common properties/attributes to common values. It's fairly easy to check that things were set correctly - in the case of img.width, when setting fails the value of img.width remains null.

If anyone has some time to put together even a JSFiddle or something, that would be a great way to get a bit more confidence and data into the issue! :)

[enapupe]

Am I making the right assumption thinking that we should also implement all these tests https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js ? I can do that.

And I believe the idea starts from something like http://jsbin.com/yoxelehufa/edit?js,output
(from https://github.com/alexmingoia/html-attributes). It maps camelCased names to attribute names, like:
"className": "class",

Then we have to add values to each of them, I added just one example on width.

[developit]

We can't add any mappings to Preact due to the filesize constraint. That likely means some of the tests you linked to won't be exactly as they are in React - the values will still work, but only becuase the DOM reflected properties apply similar behavior to React.

If you meant for tests though, a mapping might be nice. We'll just have to individually look at any failures and see if they are supportable cases or just accepted differences.

[enapupe]

So.. I'm sure you are aware, but React made a big move recently on this subject. How does that affects preact?
I'm taking about (mostly) https://github.com/facebook/react/pull/10385/files and https://facebook.github.io/react/blog/2017/09/08/dom-attributes-in-react-16.html

They also did some great work on mapping everything https://github.com/facebook/react/blob/master/fixtures/attribute-behavior/AttributeTableSnapshot.md
I wonder how much filesize did they shaved by ditching some of the maps..

[developit]

Sorry for dropping this on the floor!

React's change helps, but we're still stick because 'width' in img is true.

[marvinhagemeister]

After a short discussion we came to the conclusion that we won't support setting percentage values in width or height HTML attributes. @robertknight checked with The HTML spec and it turns out that only pixel values are supported. Seeing percentages is this not spec compliant and CSS should be used for that purpose instead 👍