-
-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.setProperty
should not work with camelCase
#61
Conversation
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 jsdom#60
Hello? |
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 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; |
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 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? |
@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 |
@jsakas Correct me if I'm wrong, but the // 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. |
Bump @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 |
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? |
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. |
No problem! I'll see if I can find one too when I have some more spare time. Keep me updated! |
Thought this comment could be relevant to this conversation, so adding it here as a reference: mdn/data#149 (comment) |
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) ? |
@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. |
@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 Let me know what you think. |
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
Awesome @jsakas ! Thank you too for your support ! |
According to the official spec,
CSSStyleDeclaration.setProperty()
'spropertyName
parameter should be the CSS property name in hyphen-caseonly. 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.