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.

Sign up for GitHub

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

Jump to bottom

Shorthand inline CSS properties not saved correctly in the styleObj #1246

Closed
wants to merge 4 commits into from
Closed

Shorthand inline CSS properties not saved correctly in the styleObj #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb604b5
Respect shorthand properties when generating attributes
okejminja Jun 29, 2023
a7d7099
Update snapshot & run tests
okejminja Jun 29, 2023
c47332f
Removed unused import
okejminja Jun 29, 2023
48fec9f
Ran prettier
okejminja Jun 29, 2023
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
6 changes: 6 additions & 0 deletions packages/rrweb-snapshot/test/__snapshots__/integration.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,12 @@ exports[`integration tests [html file]: picture-in-frame.html 1`] = `
</body></html>"
`;

exports[`integration tests [html file]: picture-with-inline-onload.html 1`] = `
"<html xmlns=\\"http://www.w3.org/1999/xhtml\\"><head></head><body>
<img src=\\"http://localhost:3030/images/robot.png\\" alt=\\"This is a robot\\" style=\\"opacity: 1;\\" _onload=\\"this.style.opacity=1\\" />
</body></html>"
`;

exports[`integration tests [html file]: preload.html 1`] = `
"<!DOCTYPE html><html lang=\\"en\\"><head>
<meta charset=\\"UTF-8\\" />
Expand Down
8 changes: 6 additions & 2 deletions packages/rrweb/src/record/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
isSerializedStylesheet,
inDom,
getShadowHost,
getInlineCSSProperties,
} from '../utils';

type DoubleLinkedListNode = {
Expand Down Expand Up @@ -340,13 +341,13 @@
};

while (this.mapRemoves.length) {
this.mirror.removeNodeFromMap(this.mapRemoves.shift()!);

Check warning on line 344 in packages/rrweb/src/record/mutation.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/record/mutation.ts#L344 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}

for (const n of this.movedSet) {
if (
isParentRemoved(this.removes, n, this.mirror) &&
!this.movedSet.has(n.parentNode!)

Check warning on line 350 in packages/rrweb/src/record/mutation.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/record/mutation.ts#L350 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
) {
continue;
}
Expand Down Expand Up @@ -574,7 +575,10 @@
item.attributes.style = {};
}
const styleObj = item.attributes.style as styleAttributeValue;
for (const pname of Array.from(target.style)) {
const targetStyle = target.getAttribute('style');
const oldStyle = old.getAttribute('style');
Copy link
Contributor

Choose a reason for hiding this comment

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

Note we could just get rid of the old variable on line 567 altogether, as we've already got m.oldValue corresponding to oldStyle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old variable on line 567 can be removed if you are not going to be using getPropertyValue or getPropertyPriority since it counts on CSSStyleDeclaration type and string cannot be cast as one, it has to be assigned to a style attribute first

As for me getting the style attribute on line 579, it was a mistake. I could've just used the m.oldValue


for (const pname of getInlineCSSProperties(targetStyle)) {
const newValue = target.style.getPropertyValue(pname);
const newPriority = target.style.getPropertyPriority(pname);
if (
Expand All @@ -588,7 +592,7 @@
}
}
}
for (const pname of Array.from(old.style)) {
for (const pname of getInlineCSSProperties(oldStyle)) {
if (target.style.getPropertyValue(pname) === '') {
// "if not set, returns the empty string"
styleObj[pname] = false; // delete
Expand Down
10 changes: 10 additions & 0 deletions packages/rrweb/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
'now you can use replayer.getMirror() to access the mirror instance of a replayer,' +
'\r\n' +
'or you can use record.mirror to access the mirror instance during recording.';
/** @deprecated */

Check warning on line 33 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L33 

[tsdoc/syntax] tsdoc-missing-deprecation-message: The @deprecated block must include a deprecation message, e.g. describing the recommended alternative
export let _mirror: DeprecatedMirror = {
map: {},
getId() {
Expand Down Expand Up @@ -114,7 +114,7 @@
set(value) {
// put hooked setter into event loop to avoid of set latency
setTimeout(() => {
d.set!.call(this, value);

Check warning on line 117 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L117 

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}, 0);
if (original && original.set) {
original.set.call(this, value);
Expand All @@ -127,7 +127,7 @@

// copy from https://github.com/getsentry/sentry-javascript/blob/b2109071975af8bf0316d3b5b38f519bdaf5dc15/packages/utils/src/object.ts
export function patch(
source: { [key: string]: any },

Check warning on line 130 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L130 

[@typescript-eslint/no-explicit-any] Unexpected any. Specify a different type.
name: string,
replacement: (...args: unknown[]) => unknown,
): () => void {
Expand Down Expand Up @@ -572,3 +572,13 @@
if (!doc) return false;
return doc.contains(n) || shadowHostInDom(n);
}

export function getInlineCSSProperties(value: string | null): string[] {
if (!value) {
return [];
}
return value
.split(';')
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone does style="content: 'l33t;'; background-color: orange"? Then this'll get split after l33t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, maybe there should be a more complex regex to split these values, some sort of CSSOM or even something extra crazy. Will keep thinking about this. Thanks for the feedback!

.map((declaration) => declaration.split(':')[0].trim())
.filter((declaration) => !!declaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the same thing but is neater

Suggested change
.filter((declaration) => !!declaration);
.filter(Boolean);

}
160 changes: 160 additions & 0 deletions packages/rrweb/test/__snapshots__/record.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1843,6 +1843,166 @@ exports[`record captures nested stylesheet rules 1`] = `
]"
`;

exports[`record captures shorthand style properties 1`] = `
"[
{
\\"type\\": 4,
\\"data\\": {
\\"href\\": \\"about:blank\\",
\\"width\\": 1920,
\\"height\\": 1080
}
},
{
\\"type\\": 2,
\\"data\\": {
\\"node\\": {
\\"type\\": 0,
\\"childNodes\\": [
{
\\"type\\": 1,
\\"name\\": \\"html\\",
\\"publicId\\": \\"\\",
\\"systemId\\": \\"\\",
\\"id\\": 2
},
{
\\"type\\": 2,
\\"tagName\\": \\"html\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 2,
\\"tagName\\": \\"head\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 4
},
{
\\"type\\": 2,
\\"tagName\\": \\"body\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 6
},
{
\\"type\\": 2,
\\"tagName\\": \\"input\\",
\\"attributes\\": {
\\"type\\": \\"text\\",
\\"size\\": \\"40\\"
},
\\"childNodes\\": [],
\\"id\\": 7
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\\\n \\\\n \\",
\\"id\\": 8
}
],
\\"id\\": 5
}
],
\\"id\\": 3
}
],
\\"id\\": 1
},
\\"initialOffset\\": {
\\"left\\": 0,
\\"top\\": 0
}
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [],
\\"removes\\": [],
\\"adds\\": [
{
\\"parentId\\": 4,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 2,
\\"tagName\\": \\"style\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 9
}
},
{
\\"parentId\\": 9,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 3,
\\"textContent\\": \\":root { --bg-orange: #FF3300; }\\",
\\"isStyle\\": true,
\\"id\\": 10
}
},
{
\\"parentId\\": 5,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 2,
\\"tagName\\": \\"div\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 11
}
}
]
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 11,
\\"attributes\\": {
\\"style\\": {
\\"background\\": \\"var(--bg-orange)\\"
}
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 11,
\\"attributes\\": {
\\"style\\": {
\\"background-color\\": \\"rgb(0, 0, 0)\\",
\\"background\\": false
}
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
}
]"
`;

exports[`record captures style property changes 1`] = `
"[
{
Expand Down
50 changes: 50 additions & 0 deletions packages/rrweb/test/record.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {
IncrementalSource,
styleSheetRuleData,
selectionData,
styleAttributeValue,
mutationData,
} from '@rrweb/types';
import {
assertSnapshot,
Expand Down Expand Up @@ -279,6 +281,54 @@ describe('record', function (this: ISuite) {
assertSnapshot(ctx.events);
});

it('captures shorthand style properties', async () => {
await ctx.page.evaluate(() => {
const { record } = (window as unknown as IWindow).rrweb;

record({
emit: (window as unknown as IWindow).emit,
});

const styleElement = document.createElement('style');
styleElement.innerText = ':root { --bg-orange: #FF3300; }';
document.head.appendChild(styleElement);

const div = document.createElement('div');
document.body.appendChild(div);

setTimeout(() => {
div.setAttribute('style', 'background: var(--bg-orange)');
}, 5);

setTimeout(() => {
div.setAttribute('style', 'background-color: #000000');
}, 10);
});

await ctx.page.waitForTimeout(50);

const attributeMutationEvents = ctx.events.filter(
(e) =>
e.type === EventType.IncrementalSnapshot &&
e.data.source === IncrementalSource.Mutation &&
e.data.attributes.length,
);

const expectedShorthandBackground = (
(attributeMutationEvents[0].data as mutationData)?.attributes[0]
?.attributes.style as styleAttributeValue
)?.['background'];
const expectedLonghandBackground = (
(attributeMutationEvents[1].data as mutationData)?.attributes[0]
?.attributes.style as styleAttributeValue
)?.['background'];

expect(attributeMutationEvents.length).toEqual(2);
expect(expectedShorthandBackground).toEqual('var(--bg-orange)');
expect(expectedLonghandBackground).toEqual(false);
assertSnapshot(ctx.events);
});

it('captures stylesheet rules', async () => {
await ctx.page.evaluate(() => {
const { record } = (window as unknown as IWindow).rrweb;
Expand Down