Use textContent of stylesheet if available #61
Merged
Conversation
This file contains 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
This will fix a bug where our style collection would not work if the stylesheet was using this pattern: font: var(--some-var); font-weight: var(--some-other-var); Iterating over cssRules here would mean we collected styles that looked something like: font-style: ; font-variant-ligatures: ; font-variant-caps: ; font-variant-numeric: ; font-variant-east-asian: ; font-variant-alternates: ; font-variant-position: ; font-variant-emoji: ; font-stretch: ; font-size: ; line-height: ; font-family: ; font-optical-sizing: ; font-size-adjust: ; font-kerning: ; font-feature-settings: ; font-variation-settings: ; font-weight: var(--some-other-var); Notice how properties are expanded and left with no content. To fix this, we can use `textContent` of the style element instead. And if that isn't available, we fall back to `sheet.cssRules`. The only potential issue this could introduce is if you mix text content styles and dynamically injected ones (using e.g. `insertRule`). I doubt that's a common way to create styling however.
lencioni
approved these changes
Feb 3, 2025
takeDOMSnapshot.js
Outdated
| .split('\n') | ||
| .map((line) => line.trim()) | ||
| .filter((line) => line && !COMMENT_PATTERN.test(line)) | ||
| .join(' '); |
There was a problem hiding this comment.
On line 16 we join with \n. Should we do the same here?
takeDOMSnapshot.js
Outdated
Comment on lines
44
to
45
| ? stripComments(element.textContent) | ||
| : getContentFromStyleSheet(element.sheet); |
There was a problem hiding this comment.
The naming of these functions seems a bit off to me. Also, I think perhaps it should be refactored a bit where here and below we would call getContentFromStyleSheet(element) and then that function would be this:
function getContentFromStyleSheet(element) {
const lines = element.textContent
? element.textContent.split('\n').map(line => line.trim())
: element.sheet.map((rule) => rule.cssText);
return lines
.filter((line) => line && !COMMENT_PATTERN.test(line))
.join('\n');
}That sorts out the naming and avoids duplicating/forking logic.
There was a problem hiding this comment.
Oh it looks like adoptedStyleSheets will need slightly different logic.
Comment on lines
66
to
67
| expect(snapshot.cssBlocks[0].content).toMatch(/font-weight: 400;/); | ||
| expect(snapshot.cssBlocks[0].content).toMatch(/font: 400 1rem/); |
There was a problem hiding this comment.
Should we do something here to make it more obvious that these are the variables? We could use names that don't match the styles exactly, and also maybe include the leading -- in the assertions?
|
I'll make some small changes here. |
I found the naming here to be a bit off, and I didn't like that the logic was duplicated and different. This commit attempts to refactor this code to have better naming and logical flow.
We had to make some small updates because we're no longer one-lining everything. I also took the time to name the custom vars better so that the assertions are easier to make.
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
This will fix a bug where our style collection would not work if the stylesheet was using this pattern:
font: var(--some-var);
font-weight: var(--some-other-var);
Iterating over cssRules here would mean we collected styles that looked something like:
font-style: ;
font-variant-ligatures: ;
font-variant-caps: ;
font-variant-numeric: ;
font-variant-east-asian: ;
font-variant-alternates: ;
font-variant-position: ;
font-variant-emoji: ;
font-stretch: ;
font-size: ;
line-height: ;
font-family: ;
font-optical-sizing: ;
font-size-adjust: ;
font-kerning: ;
font-feature-settings: ;
font-variation-settings: ;
font-weight: var(--some-other-var);
Notice how properties are expanded and left with no content.
To fix this, we can use
textContentof the style element instead. And if that isn't available, we fall back tosheet.cssRules.The only potential issue this could introduce is if you mix text content styles and dynamically injected ones (using e.g.
insertRule). I doubt that's a common way to create styling however.