Handle empty string and null for margin and padding subparts #165

andrewiggins · 2023-11-10T01:20:04Z

Per the linked issue in jsdom, the following bug exists, where dom is a JSDom instance:

const elem = dom.window.document.createElement('a');

elem.style.marginTop = '10px';
console.log(elem.style.marginTop); // '10px'

elem.style.marginTop = '';
console.log(elem.style.marginTop); // Expected '', but '10px' received.

This PR fixes this bug by specifying that null and empty string are valid values for subparts of the properties margin and padding. I've also added tests for the behavior of setting these properties to undefined (value should remain as is).

Verified that this behavior (using the repro above) matches the following browser's behavior:

Chrome 119.0.6045.123
Edge 120.0.2186.2
Firefox 120.0b8
Safari 17.1 (19616.2.9.11.7)

Fixes jsdom/jsdom#2504

domenic

Thanks, the tests make this pretty easy to accept.

When reading the parent node's styles during resize, we can sometimes encounter styles with the value ''. This can happen if the node is removed from the DOM very quickly after mount (window.getComputedStyle returns '' for elements not attached to DOM). This was not handled by null-coalescing, so we need to switch to use falsy check instead. This appears to be the same issue from bvaughn/react-virtualized#150. Tests are not included - it looks like our JSDOM version doesn't support empty string values for css properties, as far as I can tell (See jsdom/jsdom#2504 and jsdom/cssstyle#165)

When reading the parent node's styles during resize, we can sometimes encounter styles with the value ''. This can happen if the node is removed from the DOM very quickly after mount (window.getComputedStyle returns '' for elements not attached to DOM). This was not handled by null-coalescing, so we need to switch to use falsy check instead. This appears to be the same issue from bvaughn/react-virtualized#150. Tests are not included - it looks like our JSDOM version doesn't support empty string values for css properties, as far as I can tell (See jsdom/jsdom#2504 and jsdom/cssstyle#165) Fixes bvaughn#78

andrewiggins added 2 commits November 10, 2023 00:47

Setting empty string to margin or padding parts should clear property

504df22

Add support for null and add tests for undefined

d5c0105

andrewiggins changed the title ~~Margin empty string~~ Handle empty string and null for margin and padding subparts Nov 10, 2023

Fix lint errors

6629884

domenic approved these changes Dec 28, 2023

View reviewed changes

domenic merged commit 792877d into jsdom:master Dec 28, 2023
4 checks passed

andrewiggins deleted the margin-empty-string branch January 4, 2024 20:54

saperdadsk mentioned this pull request Feb 2, 2024

Handle empty string values on reading parent node style bvaughn/react-virtualized-auto-sizer#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle empty string and null for margin and padding subparts #165

Handle empty string and null for margin and padding subparts #165

andrewiggins commented Nov 10, 2023

domenic left a comment

Handle empty string and null for margin and padding subparts #165

Handle empty string and null for margin and padding subparts #165

Conversation

andrewiggins commented Nov 10, 2023

domenic left a comment

Choose a reason for hiding this comment