Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: grid template is parsed incorrectly by chrome #1396

Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
58 changes: 51 additions & 7 deletions packages/rrweb-snapshot/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import {
documentNode,
documentTypeNode,
elementNode,
idNodeMap,
IMirror,
MaskInputFn,
MaskInputOptions,
nodeMetaMap,
IMirror,
serializedNodeWithId,
serializedNode,
NodeType,
documentNode,
documentTypeNode,
serializedNode,
serializedNodeWithId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution!
Small note: reordering here will make it harder to merge in other in-progress PRs that might make changes in the same lines and makes it harder to review; looks like there are no changes?

textNode,
elementNode,
} from './types';

export function isElement(n: Node): n is Element {
Expand Down Expand Up @@ -71,7 +71,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 74 in packages/rrweb-snapshot/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

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

[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 @@ -106,8 +106,48 @@
}
}

function replaceChromeGridTemplateAreas(rule: CSSStyleRule): string {
const hasGridTemplateInCSSText = rule.cssText.includes('grid-template:');
const hasGridTemplateAreaInStyleRules =
rule.style.getPropertyValue('grid-template-areas') !== '';
const hasGridTemplateAreaInCSSText = rule.cssText.includes(
'grid-template-areas:',
);
if (
isCSSStyleRule(rule) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This isCSSStyleRule check has already been performed outside of the function call

hasGridTemplateInCSSText &&
hasGridTemplateAreaInStyleRules &&
!hasGridTemplateAreaInCSSText
) {
// chrome does not correctly provide the grid template areas in the rules cssText
// e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=1303968
// we remove the grid-template rule from the text... so everything from grid-template: to the next semicolon
// and then add each grid-template-x rule into the css text because Chrome isn't doing this correctly
const parts = rule.cssText
.split(';')
.filter((s) => !s.includes('grid-template:'))
.map((s) => s.trim());

const gridStyles: string[] = [];

for (let i = 0; i < rule.style.length; i++) {
const styleName = rule.style[i];
if (styleName.startsWith('grid-template')) {
gridStyles.push(
`${styleName}: ${rule.style.getPropertyValue(styleName)}`,
);
}
}
parts.splice(parts.length - 1, 0, gridStyles.join('; '));
return parts.join('; ');
}
return rule.cssText;
}

export function stringifyRule(rule: CSSRule): string {
let importStringified;
let gridTemplateFixed;

if (isCSSImportRule(rule)) {
try {
importStringified =
Expand All @@ -125,7 +165,11 @@
return fixSafariColons(rule.cssText);
}

return importStringified || rule.cssText;
if (isCSSStyleRule(rule)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check will be performed twice at this point; we create one } else if (isCSSStyleRule(rule)) { section and have this grid fix and the safari colons fix within it

gridTemplateFixed = replaceChromeGridTemplateAreas(rule);
}

return importStringified || gridTemplateFixed || rule.cssText;
}

export function fixSafariColons(cssStringified: string): string {
Expand Down
52 changes: 51 additions & 1 deletion packages/rrweb-snapshot/test/css.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { parse, Rule, Media } from '../src/css';
import { fixSafariColons, escapeImportStatement } from './../src/utils';
import {
fixSafariColons,
escapeImportStatement,
stringifyRule,
} from './../src/utils';

describe('css parser', () => {
it('should save the filename and source', () => {
Expand Down Expand Up @@ -119,6 +123,52 @@ describe('css parser', () => {
expect(out3).toEqual('[data-aa\\:other] { color: red; }');
});

it('does not alter correctly parsed grid template rules', () => {
const cssText =
'#wrapper { display: grid; width: 100%; height: 100%; grid-template: repeat(2, 1fr); margin: 0px auto; }';
const stringified = stringifyRule({
cssText: cssText,
} as Partial<CSSStyleRule> as CSSStyleRule);
expect(stringified).toEqual(cssText);
});

it('fixes incorrectly parsed grid template rules', () => {
const cssText =
'#wrapper { display: grid; grid-template: "header header" max-content / repeat(2, 1fr); margin: 0px auto; }';
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be valid grid-template syntax

"Note: The repeat() function isn't allowed in these track listings, as the tracks are intended to visually line up one-to-one with the rows/columns in the "ASCII art"."
https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template#values

// to avoid using JSDom we can fake as much of the CSSStyleDeclaration as we need
const cssStyleDeclaration: Record<string | number, any> = {
length: 3,
0: 'grid-template-areas',
1: 'grid-template-rows',
2: 'grid-template-columns',
items: (i: number): string => {
return [cssStyleDeclaration[i]].toString();
},
getPropertyValue: (key: string): string => {
if (key === 'grid-template-areas') {
return '"header header" "main main" "footer footer"';
}
if (key === 'grid-template-rows') {
return 'repeat(2, 1fr)';
}
if (key === 'grid-template-columns') {
return 'repeat(2, 1fr)';
}
return '';
},
};

const stringified = stringifyRule({
cssText: cssText,
selectorText: '#wrapper',
style: cssStyleDeclaration as unknown as CSSStyleDeclaration,
} as Partial<CSSStyleRule> as CSSStyleRule);

expect(stringified).toEqual(
'#wrapper { display: grid; margin: 0px auto; grid-template-areas: "header header" "main main" "footer footer"; grid-template-rows: repeat(2, 1fr); grid-template-columns: repeat(2, 1fr); }',
);
});

it('parses imports with quotes correctly', () => {
const out1 = escapeImportStatement({
cssText: `@import url("/foo.css;900;800"");`,
Expand Down
Loading