Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
Prev Previous commit
Next Next commit
Solve the same bug another way; by normalization
  • Loading branch information
eoghanmurray committed Jan 29, 2025
commit 01d96bbfca61eedcadaab0df8390b36639daffe6
15 changes: 10 additions & 5 deletions packages/rrweb-snapshot/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
* Browsers sometimes incorrectly escape `@import` on `.cssText` statements.
* This function tries to correct the escaping.
* more info: https://bugs.chromium.org/p/chromium/issues/detail?id=1472259
* @param cssImportRule

Check warning on line 83 in packages/rrweb-snapshot/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/utils.ts#L83

[tsdoc/syntax] tsdoc-param-tag-missing-hyphen: The @param block should be followed by a parameter name and then a hyphen
* @returns `cssText` with browser inconsistencies fixed, or null if not applicable.
*/
export function escapeImportStatement(rule: CSSImportRule): string {
Expand Down Expand Up @@ -450,8 +450,12 @@
* Intention is to normalize by remove spaces, semicolons and CSS comments
* so that we can compare css as authored vs. output of stringifyStylesheet
*/
export function normalizeCssString(cssText: string): string {
return cssText.replace(/(\/\*[^*]*\*\/)|[\s;]/g, '');
export function normalizeCssString(cssText: string, _testNoPxNorm=false): string {
if (_testNoPxNorm) {
return cssText.replace(/(\/\*[^*]*\*\/)|[\s;]/g, '');
} else {
return cssText.replace(/(\/\*[^*]*\*\/)|[\s;]/g, '').replace(/0px/g, '0');
Copy link
Contributor

Choose a reason for hiding this comment

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

i briefly tested a very naive loop parser and it was slower than regex replace - i guess because browsers/v8 are doing some magic to optimise this already

but I didn't test it over a range of inputs

this is (from my testing) at best O(n) for whitespace - and for clarity since i'm not much of a comp sci person. if you insert whitespace into the input then this gets slower the more whitespace is present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The performance of the normalization function could be improved. I've moreso tried to ensure it's not called repeatedly on the same piece of css (with the 'binary search' style changes in #1615 ).

}
}

/**
Expand All @@ -463,19 +467,20 @@
export function splitCssText(
cssText: string,
style: HTMLStyleElement,
_testNoPxNorm=false,
): string[] {
const childNodes = Array.from(style.childNodes);
const splits: string[] = [];
let iterLimit = 0;
if (childNodes.length > 1 && cssText && typeof cssText === 'string') {
let cssTextNorm = normalizeCssString(cssText);
let cssTextNorm = normalizeCssString(cssText, _testNoPxNorm);
const normFactor = cssTextNorm.length / cssText.length;
for (let i = 1; i < childNodes.length; i++) {
if (
childNodes[i].textContent &&
typeof childNodes[i].textContent === 'string'
) {
const textContentNorm = normalizeCssString(childNodes[i].textContent!);
const textContentNorm = normalizeCssString(childNodes[i].textContent!, _testNoPxNorm);

Check warning on line 483 in packages/rrweb-snapshot/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/utils.ts#L483

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
let j = 3;
for (; j < textContentNorm.length; j++) {
if (
Expand Down Expand Up @@ -538,7 +543,7 @@
splits.push(cssText);
return splits;
}
const normPart = normalizeCssString(cssText.substring(0, k));
const normPart = normalizeCssString(cssText.substring(0, k), _testNoPxNorm);
if (normPart.length === splitNorm) {
splits.push(cssText.substring(0, k));
cssText = cssText.substring(k);
Expand Down
7 changes: 5 additions & 2 deletions packages/rrweb-snapshot/test/css.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ describe('css splitter', () => {
// happydom? bug avoid: strangely a greater than symbol in the template string below
// e.g. '.prose > :last-child' causes more than one child to be appended
style.append(`ass~="not-prose"] *)) {
margin-top: 0; /* this line triggers the bug as we would do a simpler match if it was authored as 'margin-top: 0px;' */
margin-top: 0; /* cssRules transforms this to '0px' which was preventing matching prior to normalization */
}

.section-news-v3-detail .news-cnt-wrapper .plugins-wrapper2 :where(.prose :last-child):not(:where([class~="not-prose"],[class~="not-prose"] *)) {
Expand All @@ -273,7 +273,10 @@ describe('css splitter', () => {
];
const browserSheet = expected.join('');
expect(stringifyStylesheet(style.sheet!)).toEqual(browserSheet);
expect(splitCssText(browserSheet, style)).toEqual(expected);
let _testNoPxNorm = true; // trigger the original motivating scenario for this test
expect(splitCssText(browserSheet, style, _testNoPxNorm)).toEqual(expected);
_testNoPxNorm = false; // this case should also be solved by normalizing '0px' -> '0'
expect(splitCssText(browserSheet, style, _testNoPxNorm)).toEqual(expected);
}
});

Expand Down
Loading