Add a check for undefined val for parseColor #190
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you explain why you are passing undefined to this function in the first place? |
I'm not - Stencil JS is. When running test for web components built with Stencil that line under some circumstances causes After looking both cssColor and Stencil, I feel cssStlyle is the right place to fix this. |
|
It's probably wrong to treat See the test case below. Note that when So the right approach might be to add a try-catch around try {
var res = resolveColor(val, {
format: 'specifiedValue',
});
if (res) {
return res;
}
} catch (e) {
// Return the previous value somehow
}
return undefined;But I'm not sure if we are caching values somewhere. Test case: <!DOCTYPE html>
<html>
<style>
.div {
background-color: red;
width: 50%;
height: 3rem;
}
</style>
<div id="div" class="div"></div>
<script>
const declaration = document.styleSheets[0].cssRules[0].style;
let declarationValue = declaration.getPropertyValue('background-color');
console.log(`
declarationValue: ${declarationValue}
`);
declaration.setProperty('background-color', 'green');
declarationValue = declaration.getPropertyValue('background-color');
console.log(`
declaration.setProperty('background-color', 'green');
declarationValue: ${declarationValue}
`);
declaration.setProperty('background-color', undefined);
declarationValue = declaration.getPropertyValue('background-color');
console.log(`
declaration.setProperty('background-color', undefined);
declarationValue: ${declarationValue}
`);
const div = document.getElementById('div');
let propertyValue = getComputedStyle(div).getPropertyValue('background-color');
let stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
default value:
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}, is empty string: ${stylePropertyValue === ''}
`);
div.style.setProperty('background-color', 'blue');
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.setProperty('background-color', 'blue');
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}
`);
div.style.setProperty('background-color', undefined);
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.setProperty('background-color', undefined);
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}
`);
div.style.setProperty('background-color', '');
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.setProperty('background-color', '');
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}, is empty string: ${stylePropertyValue === ''}
`);
div.style.backgroundColor = 'rebeccapurple';
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.backgroundColor = 'rebeccapurple';
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}
`);
div.style.backgroundColor = undefined;
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.backgroundColor = undefined;
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}
`);
div.style.backgroundColor = null;
propertyValue = getComputedStyle(div).getPropertyValue('background-color');
stylePropertyValue = div.style.getPropertyValue('background-color');
console.log(`
div.style.backgroundColor = null;
propertyValue: ${propertyValue}
stylePropertyValue: ${stylePropertyValue}, is empty string: ${stylePropertyValue === ''}
`);
</script>
</html>Results in browser: declarationValue: red
declaration.setProperty('background-color', 'green');
declarationValue: green
declaration.setProperty('background-color', undefined);
declarationValue: green
default value:
propertyValue: rgb(0, 128, 0)
stylePropertyValue: , is empty string: true
div.style.setProperty('background-color', 'blue');
propertyValue: rgb(0, 0, 255)
stylePropertyValue: blue
div.style.setProperty('background-color', undefined);
propertyValue: rgb(0, 0, 255)
stylePropertyValue: blue
div.style.setProperty('background-color', '');
propertyValue: rgb(0, 128, 0)
stylePropertyValue: , is empty string: true
div.style.backgroundColor = 'rebeccapurple';
propertyValue: rgb(102, 51, 153)
stylePropertyValue: rebeccapurple
div.style.backgroundColor = undefined;
propertyValue: rgb(102, 51, 153)
stylePropertyValue: rebeccapurple
div.style.backgroundColor = null;
propertyValue: rgb(0, 128, 0)
stylePropertyValue: , is empty string: true |
|
I've looked into it a bit deeper and it seems there's nothing wrong with returning undefined early. |
|
I guess I am not excited to merge this until we can get a test in the upstream jsdom repository that this solves. I also wonder if this is just one instance of #129. |
|
Test summary: #196 (comment) |
domenic
pushed a commit
to jsdom/jsdom
that referenced
this pull request
Apr 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue #189