jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface
MIT License
109 stars 70 forks source link

An alternative fix for PR#47 - Add support for numeric values passed to ‘border-spacing’, e.g. ‘border-spacing: 0’ #52

Closed lsiu closed 6 years ago

lsiu commented 7 years ago

This fix include a test and follow the approach used in padding.js

eddies commented 6 years ago

@lsiu can you rebase this on master? Hopefully that would increase the chances of getting this merged. I have a rebased version of your branch ready at https://github.com/eddies/CSSStyleDeclaration/tree/pr-47-rebase, but I don't want to duplicate your PR

lsiu commented 6 years ago

@eddies - I rebased with upstream/master for this pull request. Now there is no merge conflict. @jsakas - Seems like a few folks are having the the same problem. Any chance for this PR getting accepted? Any feedback is also appreciated. Thanks.

lsiu commented 6 years ago

If you refer to the w3c border-spacing spec, you can see the "initial value" is 0, not "0", so I suppose a numerical value is also a valid, and should be taken into consideration.

jsakas commented 6 years ago

@lsiu @eddies @jansiegel

Browsers do not seem to support this functionality. Tested in Chrome, FF, Safari, Edge:

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 10);
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', '10');
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', '10px');
console.log(table.style.borderSpacing); // 10px

Instead of converting the value, I think it would make more sense to return if the value is not a string.

eddies commented 6 years ago

@jsakas In Firefox 61.0 (OS X):

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 10);
console.log(table.style.borderSpacing); // "10px"
table.style.setProperty('border-spacing', '10');
console.log(table.style.borderSpacing); // "10px"

Moreover, in Firefox, Chrome (67) and Safari (11.1.1), I observe the following:

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 0);
console.log(table.style.borderSpacing); // "0px"
table.style.setProperty('border-spacing', '0');
console.log(table.style.borderSpacing); // "0px"

So I don't think a blanket return undefined is the clear winner. I think it's either the PR as-is or a special case for 0 and undefined for other numbers.

jsakas commented 6 years ago

@eddies interesting, it seems to work for 0 only, but not for any other integer.