Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use textContent of stylesheet if available #61

Merged
merged 3 commits into from
Feb 3, 2025
Merged

Use textContent of stylesheet if available #61

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe2ef46
Use textContent of stylesheet if available
trotzig Feb 3, 2025
29c42dd
Refactor getContentFromStylesheet to avoid duplication
lencioni Feb 3, 2025
e0f3470
Fix tests for new css extraction
trotzig Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions playwright-tests/takeDOMSnapshot.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,28 @@ test('regular elements', async ({ page }) => {
baseUrl: 'http://localhost:7700/regular-elements',
},
]);

expect(snapshot.cssBlocks).toEqual([]);
});

test('style collection', async ({ page }) => {
await setupPage(page);

await page.goto('/style-collection');

const snapshot = await page.evaluate(() => {
return window.happoTakeDOMSnapshot({ doc: document, element: document.body });
});

expect(snapshot.cssBlocks.length).toBe(2);
expect(snapshot.cssBlocks[0].content).toMatch(/font-weight: 400;/);
expect(snapshot.cssBlocks[0].content).toMatch(/font: 400 1rem/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

expect(snapshot.cssBlocks[0].content).toMatch(/font: var\(--font\)/);
expect(snapshot.cssBlocks[0].content).toMatch(
/font-weight: var\(--font-weight\);/,
);

expect(snapshot.cssBlocks[1].content).toMatch(/color: yellow;/);
});

test('one custom element', async ({ page }) => {
Expand Down
19 changes: 19 additions & 0 deletions playwright-tests/test-assets/style-collection/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<html>
<head>
<style type="text/css">
:root {
--font: 400 1rem / 1.5rem Roboto, Arial, Helvetica, sans-serif;
--font-weight: 400;
}
p {
font: var(--font);
font-weight: var(--font-weight);
}
</style>
<style type="text/css" id="dynamic-style"></style>
</head>
<body>
<main>Hello world</main>
</body>
<script src="/style-collection/index.js"></script>
</html>
3 changes: 3 additions & 0 deletions playwright-tests/test-assets/style-collection/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
document.head
.querySelector('#dynamic-style')
.sheet.insertRule('main { color: yellow; }');
43 changes: 27 additions & 16 deletions takeDOMSnapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,25 @@ const findCSSAssetUrls = require('./src/findCSSAssetUrls');
const CSS_ELEMENTS_SELECTOR = 'style,link[rel="stylesheet"][href]';
const COMMENT_PATTERN = /^\/\*.+\*\/$/;

function getContentFromStyleSheet(styleSheet) {
return (
Array.from(styleSheet.cssRules)
.map((rule) => rule.cssText)
// Filter out those lines that are comments (these are often source
// mappings)
.filter((line) => !COMMENT_PATTERN.test(line))
.join('\n')
);
}

function stripComments(content) {
return content
.split('\n')
.map((line) => line.trim())
.filter((line) => line && !COMMENT_PATTERN.test(line))
.join(' ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 16 we join with \n. Should we do the same here?

}

function extractCSSBlocks(doc) {
const blocks = [];
const styleElements = doc.querySelectorAll(CSS_ELEMENTS_SELECTOR);
Expand All @@ -21,25 +40,19 @@ function extractCSSBlocks(doc) {
const href = element.href || element.getAttribute('href');
blocks.push({ key: href, href, baseUrl: element.baseURI });
} else {
// <style>
const lines = Array.from(element.sheet.cssRules).map((r) => r.cssText);

// Filter out those lines that are comments (these are often source
// mappings)
const content = lines.filter((line) => !COMMENT_PATTERN.test(line)).join('\n');

const content = element.textContent
? stripComments(element.textContent)
: getContentFromStyleSheet(element.sheet);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it looks like adoptedStyleSheets will need slightly different logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this PR

// Create a hash so that we can dedupe equal styles
const key = md5(content).toString();
blocks.push({ content, key, baseUrl: element.baseURI });
}
});

(doc.adoptedStyleSheets || []).forEach((sheet) => {
const rules = Array.from(sheet.cssRules)
.map((r) => r.cssText)
.join('\n');
const key = md5(rules).toString();
blocks.push({ key, content: rules, baseUrl: sheet.href || document.baseURI });
const content = getContentFromStyleSheet(sheet);
const key = md5(content).toString();
blocks.push({ key, content, baseUrl: sheet.href || document.baseURI });
});
return blocks;
}
Expand Down Expand Up @@ -272,10 +285,8 @@ function inlineShadowRoots(element) {
for (const styleSheet of element.shadowRoot.adoptedStyleSheets) {
const styleElement = document.createElement('style');
styleElement.setAttribute('data-happo-inlined', 'true');
const rules = Array.from(styleSheet.cssRules)
.map((rule) => rule.cssText)
.join('\n');
styleElement.textContent = rules;
const styleContent = getContentFromStyleSheet(styleSheet);
styleElement.textContent = styleContent;
hiddenElement.appendChild(styleElement);
}

Expand Down