-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'rrweb-snapshot': patch | ||
--- | ||
|
||
adds a correction for when chrome passes an incorrect grid-template-area cssText during snapshot |
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, | ||
textNode, | ||
elementNode, | ||
} from './types'; | ||
|
||
export function isElement(n: Node): n is Element { | ||
|
@@ -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 GitHub Actions / ESLint Report Analysispackages/rrweb-snapshot/src/utils.ts#L74
|
||
* @returns `cssText` with browser inconsistencies fixed, or null if not applicable. | ||
*/ | ||
export function escapeImportStatement(rule: CSSImportRule): string { | ||
|
@@ -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) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
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 = | ||
|
@@ -125,7 +165,11 @@ | |
return fixSafariColons(rule.cssText); | ||
} | ||
|
||
return importStringified || rule.cssText; | ||
if (isCSSStyleRule(rule)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check will be performed twice at this point; we create one |
||
gridTemplateFixed = replaceChromeGridTemplateAreas(rule); | ||
} | ||
|
||
return importStringified || gridTemplateFixed || rule.cssText; | ||
} | ||
|
||
export function fixSafariColons(cssStringified: string): string { | ||
|
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', () => { | ||
|
@@ -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; }'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't appear to be valid "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"." |
||
// 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"");`, | ||
|
There was a problem hiding this comment.
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?