.setProperty should not work with camelCase

[reyronald]

According to the official spec, CSSStyleDeclaration.setProperty()'s
propertyName parameter should be the CSS property name in hyphen-case
only. It shouldn't set any styles when the property name is in
camel case or any other form. This is the behavior found in browsers,
so this library should align with it.

https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty

Closes #60

---

This is more of an attempt to get the ball rolling on fixing #60, than an actual finished proper solution. I'm obviously open to suggestions on getting the code to how it needs to be.

[reyronald]

Hello?

[jsakas]

Hey @reyronald, thanks for the PR! Apologies for the delay, I was not receiving notifications for this for some reason (fixed now!)

I have reviewed the code and also the original jsdom issue. It seems that browsers will apply a lowercase transform to the propertyName before setting it, so we need a more robust check inside of setProperty. isCamelCase('FoNt-SiZe') returns true for what is technically a valid property.

With a proper implementation we should be able to assert:

style.setProperty('FoNt-SiZe', '12px');
test.equal(style.fontSize, '12px', 'font-size: 12px');
test.equal(style.getPropertyValue('font-size'), '12px', 'font-size: 12px');

Tested in Chrome, Firefox and Safari console:

var div = document.createElement('div');
div.style.setProperty('FonT-SiZe', '12px');
console.log(div.style.fontSize); // 12px
console.log(div.style.getPropertyValue('font-size')); // 12px;

[reyronald]

Hey @jsakas ,you are absolutely right, what a great catch! Hadn't noticed that mysef.

I think the correct approach here would be to check if the given property name is a valid one directly, instead of using workarounds with utility functions like that. I mean something like this:

// `validProperties` would be a `new Set(['align-content', 'align-items', ... ])`
// only containing the dashed property names
var lowercaseName = name.toLowerCase();
if (!validProperties.has(lowercaseName)) {
	return;
}
// ...

We could generate that set along with the property setters & getter using the generate_properties.js script, it should be straight forward and minimal. We could then also re-use that set in the test file and then we would have a single source of truth for the property list, instead of having to redefine it there! That would allow us to remove line 11 from the following snippet and just import it instead.

https://github.com/jsakas/CSSStyleDeclaration/blob/a8c2467b5a2ca12a574c2eeabe5ca1205fd1aeaf/tests/tests.js#L11-L14

I think this is what would be the most consistent with what the browser is doing, and your suggested test would pass (along with all others). Do you see any downsides with it, or have a different approach?

[jsakas]

@reyronald I agree that we need to do a property name check. I think we can use MDN's data repository here: https://github.com/mdn/data/blob/master/css/properties.json

[reyronald]

@jsakas Correct me if I'm wrong, but the CSSStyleDeclaration class requires setters and getters for each property to be applied properly, and those are defined in ./lib/properties. If we grab them from that json we would be letting pass as valid some that won't actually be able to be set. This is the case of the align-self property:

// with cssstyle

var style = new cssstyle.CSSStyleDeclaration();
style.setProperty('align-self', 'auto');
console.log(style.cssText); // '', because there are no accessors for the `alignSelf` property 
// (but there should be!)

// in the browser

var div = document.createElement('div')
div.style.setProperty('align-self', 'auto');
console.log(div.outerHTML); // '<div style="align-self: auto;"></div>' as expected

We could use that json to identify the getters and setters that we are missing and add them, but not use it directly for the validity check. I'm not sure if I'm getting my point across very well, thoughts?

Let me know if I'm missing something, I might be wrong.

[reyronald]

Bump @jsakas

[jsakas]

@reyronald ideally we would use that data to generate this entire library, but that is what we should do for the re-write.

In the interim we could easily use the JSON to check valid properties. Here is some untested pseudo code:

const properties = require('mdn/data/css/properties.json');

const validProperties = Object.keys(properties);

const isValidProperty = validPropertes.indexOf(someVar) > - 1;

Also, because there are custom properties like --my-custom-prop, we might need to use regex, or treat --* as a special case.

[reyronald]

So I did an intersection between the CSS properties that we currently have in this repo and the ones exposed by the MDN data json, and there are tons of that are missing (261 to be exact), which leads me to believe that maybe that data is not actually complete and perhaps it wouldn't be a good idea to use that to generate this entire library :/. Here's a snippet with the exact list: https://gist.github.com/reyronald/cc67f9c3bb02449e88f13c46ef28a92b.

There are some tests that are relying on some of those properties, so they fail with the suggested approach. Also, the code of consumers of this library will end-up breaking too if we're not careful.

Do you know of any other npm package that has more reliable/official data?

[jsakas]

Ah, well that is unfortunate. Thanks for taking the time to do more thorough research on that. I am going to look for a more complete data source or integration we can use.

[reyronald]

No problem! I'll see if I can find one too when I have some more spare time.

Keep me updated!

[reyronald]

Thought this comment could be relevant to this conversation, so adding it here as a reference: mdn/data#149 (comment)

[reyronald]

Circling back to this, do you think we should then post-pone this issue until after the rewrite, or could we go with the approach I suggested in #61 (comment) ?

[jsakas]

@reyronald I think your approach of creating a valid properties array is a good solution for now. I would maybe put it in its own file though, something like:

// validProperties.js
module.exports = [
  'azimuth',
  'background',
  'backgroundAttachment',
  // etc..
];

That way the diff will be clearer if we need to add more, and it keeps the other file a little cleaner.

[reyronald]

@jsakas Done! How you feel about it now?

By the way I used a Set instead of an array so that the check would be a lot faster, O(1) instead of O(n). Also, I used multiple .add() calls instead of the constructor that takes an iterable because the latter is not supported on IE11. On top of that, the .add calls span multiple lines so the file would be incredibly more readable anyway, using the iterable constructor would put everything in a huge, long line.

Let me know what you think.

[jsakas]

@reyronald this is looking good. Going to test today and will get back to you. I wish we had a similar document for CSS3 properties!

[jsakas]

@reyronald I pulled this down and ran some tests and it seems to be working as expecting. I am going to publish this as a MINOR version bump. Thanks for all the help with this!

[reyronald]

Awesome @jsakas ! Thank you too for your support !