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

PR fix: Add test for null attribute #66

Merged
merged 4 commits into from
Feb 27, 2023
Merged

PR fix: Add test for null attribute #66

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
d3d330a
WIP
mydea Feb 27, 2023
60df662
WIP add test
mydea Feb 27, 2023
e84233b
fix formatting
mydea Feb 27, 2023
8a2a2be
fix types that value can be `null`
mydea Feb 27, 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
2 changes: 1 addition & 1 deletion packages/rrweb-snapshot/src/rebuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ function buildNode(
continue;
}
value =
typeof value === 'boolean' || typeof value === 'number' ? '' : value;
typeof value === 'boolean' || typeof value === 'number' || value === null ? '' : value;
// attribute names start with rr_ are internal attributes added by rrweb
if (!name.startsWith('rr_')) {
const isTextarea = tagName === 'textarea' && name === 'value';
Expand Down
27 changes: 15 additions & 12 deletions packages/rrweb-snapshot/src/snapshot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,36 +238,39 @@ export function transformAttribute(
doc: Document,
tagName: string,
name: string,
value: string,
value: string | null,
maskAllText: boolean,
maskTextFn: MaskTextFn | undefined,
): string {
): string | null {
if (!value) {
Copy link
Member

@Lms24 Lms24 Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

l: should we change value's type to string | undefined | null in the function signature to reflect that it apparently can be undefined/null?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense!

Copy link
Member Author

@mydea mydea Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, it seems there is actually a longer tail to this (as a lot of types seem to depend on the return value of this being string.)

So IMHO it's correct, but let's look at this separately maybe then!

EDIT: nevermind, I ended up fixing the type!

return value;
}

// relative path in attribute
if (name === 'src' || (name === 'href' && value)) {
if (name === 'src' || name === 'href') {
return absoluteToDoc(doc, value);
} else if (name === 'xlink:href' && value && value[0] !== '#') {
} else if (name === 'xlink:href' && value[0] !== '#') {
// xlink:href starts with # is an id pointer
return absoluteToDoc(doc, value);
} else if (
name === 'background' &&
value &&
(tagName === 'table' || tagName === 'td' || tagName === 'th')
) {
return absoluteToDoc(doc, value);
} else if (name === 'srcset' && value) {
} else if (name === 'srcset') {
return getAbsoluteSrcsetString(doc, value);
} else if (name === 'style' && value) {
} else if (name === 'style') {
return absoluteToStylesheet(value, getHref());
} else if (tagName === 'object' && name === 'data' && value) {
} else if (tagName === 'object' && name === 'data') {
return absoluteToDoc(doc, value);
} else if (
maskAllText &&
['placeholder', 'title', 'aria-label'].indexOf(name) > -1
) {
return maskTextFn ? maskTextFn(value) : defaultMaskFn(value);
} else {
return value;
}

return value;
}

export function _isBlockedElement(
Expand Down Expand Up @@ -770,8 +773,8 @@ function serializeNode(
}
}

function lowerIfExists(maybeAttr: string | number | boolean): string {
if (maybeAttr === undefined) {
function lowerIfExists(maybeAttr: string | number | boolean | null | undefined): string {
if (maybeAttr === undefined || maybeAttr === null) {
return '';
} else {
return (maybeAttr as string).toLowerCase();
Expand Down
2 changes: 1 addition & 1 deletion packages/rrweb-snapshot/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export type documentTypeNode = {
};

export type attributes = {
[key: string]: string | number | boolean;
[key: string]: string | number | boolean | null;
};
export type elementNode = {
type: NodeType.Element;
Expand Down
2 changes: 1 addition & 1 deletion packages/rrweb-snapshot/typings/snapshot.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { serializedNodeWithId, INode, idNodeMap, MaskInputOptions, SlimDOMOption
export declare const IGNORED_NODE = -2;
export declare function absoluteToStylesheet(cssText: string | null, href: string): string;
export declare function absoluteToDoc(doc: Document, attributeValue: string): string;
export declare function transformAttribute(doc: Document, tagName: string, name: string, value: string, maskAllText: boolean, maskTextFn: MaskTextFn | undefined): string;
export declare function transformAttribute(doc: Document, tagName: string, name: string, value: string | null, maskAllText: boolean, maskTextFn: MaskTextFn | undefined): string | null;
export declare function _isBlockedElement(element: HTMLElement, blockClass: string | RegExp, blockSelector: string | null, unblockSelector: string | null): boolean;
export declare function needMaskingText(node: Node | null, maskTextClass: string | RegExp, maskTextSelector: string | null, unmaskTextSelector: string | null, maskAllText: boolean): boolean;
export declare function serializeNodeWithId(n: Node | INode, options: {
Expand Down
2 changes: 1 addition & 1 deletion packages/rrweb-snapshot/typings/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export type documentTypeNode = {
systemId: string;
};
export type attributes = {
[key: string]: string | number | boolean;
[key: string]: string | number | boolean | null;
};
export type elementNode = {
type: NodeType.Element;
Expand Down
199 changes: 199 additions & 0 deletions packages/rrweb/test/__snapshots__/integration.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3684,6 +3684,205 @@ exports[`record integration tests can use maskInputOptions to configure which ty
]"
`;

exports[`record integration tests handles null attribute values 1`] = `
"[
{
"type": 0,
"data": {}
},
{
"type": 1,
"data": {}
},
{
"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": "p",
"attributes": {},
"childNodes": [
{
"type": 3,
"textContent": "******** ********",
"id": 8
}
],
"id": 7
},
{
"type": 3,
"textContent": "\\n ",
"id": 9
},
{
"type": 2,
"tagName": "ul",
"attributes": {},
"childNodes": [
{
"type": 3,
"textContent": "\\n ",
"id": 11
},
{
"type": 2,
"tagName": "li",
"attributes": {},
"childNodes": [],
"id": 12
},
{
"type": 3,
"textContent": "\\n ",
"id": 13
}
],
"id": 10
},
{
"type": 3,
"textContent": "\\n ",
"id": 14
},
{
"type": 2,
"tagName": "canvas",
"attributes": {},
"childNodes": [],
"id": 15
},
{
"type": 3,
"textContent": "\\n\\n ",
"id": 16
},
{
"type": 2,
"tagName": "script",
"attributes": {},
"childNodes": [
{
"type": 3,
"textContent": "SCRIPT_PLACEHOLDER",
"id": 18
}
],
"id": 17
},
{
"type": 3,
"textContent": "\\n \\n \\n",
"id": 19
}
],
"id": 5
}
],
"id": 3
}
],
"id": 1
},
"initialOffset": {
"left": 0,
"top": 0
}
}
},
{
"type": 3,
"data": {
"source": 0,
"texts": [],
"attributes": [
{
"id": 20,
"attributes": {
"aria-label": "*****",
"id": "test-li"
}
}
],
"removes": [],
"adds": [
{
"parentId": 10,
"nextId": null,
"node": {
"type": 2,
"tagName": "li",
"attributes": {
"aria-label": "*****",
"id": "test-li"
},
"childNodes": [],
"id": 20
}
}
]
}
},
{
"type": 3,
"data": {
"source": 0,
"texts": [],
"attributes": [
{
"id": 20,
"attributes": {
"aria-label": null
}
}
],
"removes": [],
"adds": []
}
}
]"
`;

exports[`record integration tests should mask all text (except unmaskTextSelector), using maskAllText 1`] = `
"[
{
Expand Down
31 changes: 31 additions & 0 deletions packages/rrweb/test/integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,37 @@ describe('record integration tests', function (this: ISuite) {
assertSnapshot(snapshots);
});

it('handles null attribute values', async () => {
const page: puppeteer.Page = await browser.newPage();
await page.goto('about:blank');
await page.setContent(
getHtml.call(this, 'mutation-observer.html', {
maskAllInputs: true,
maskAllText: true,
}),
);

await page.evaluate(() => {
const li = document.createElement('li');
const ul = document.querySelector('ul') as HTMLUListElement;
ul.appendChild(li);

li.setAttribute('aria-label', 'label');
li.setAttribute('id', 'test-li');
});

await new Promise((resolve) => setTimeout(resolve, 100));

await page.evaluate(() => {
const li = document.querySelector('#test-li') as HTMLLIElement;
// This triggers the mutation observer with a `null` attribute value
li.removeAttribute('aria-label');
});

const snapshots = await page.evaluate('window.snapshots');
assertSnapshot(snapshots);
});

it('can record character data muatations', async () => {
const page: puppeteer.Page = await browser.newPage();
await page.goto('about:blank');
Expand Down