Skip to content

Commit

Permalink
Compact style mutation fixes and improvements (#1268)
Browse files Browse the repository at this point in the history 
* Don't use the CSSOM when there's `var()` present as it fails badly #1246

* As the CSS Object Model expands out shorthand properties, do a check on the string length before choosing which format to go for

 - this approach allows 'var()' in a styleOMValue as it's only a problem when combined with a shorthand property
 - before this change background:black; was getting expaned to 10 OM properties as follows:

'style': {
    'background-color': 'black',
    'background-image': false,
    'background-position-x': false,
    'background-position-y': false,
    'background-size': false,
    'background-repeat-x': false,
    'background-repeat-y': false,
    'background-attachment': false,
    'background-origin': false,
    'background-clip': false
}

* Updates to remainder of tests based on refined compact style mutations

* Apply suggestions from code review by: Justin Halsall <Juice10@users.noreply.github.com>

---------

Authored-by: eoghanmurray <eoghan@getthere.ie>
  • Loading branch information
@eoghanmurray
eoghanmurray authored Aug 3, 2023
1 parent c6600e7 commit d872d28
Show file tree
Hide file tree
Showing 7 changed files with 366 additions and 72 deletions.
9 changes: 9 additions & 0 deletions .changeset/clean-plants-play.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
'rrweb': patch
'@rrweb/types': patch
---

Compact style mutation fixes and improvements

- fixes when style updates contain a 'var()' on a shorthand property #1246
- further ensures that style mutations are compact by reverting to string method if it is shorter
93 changes: 55 additions & 38 deletions packages/rrweb/src/record/mutation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import type {
attributeCursor,
removedNodeMutation,
addedNodeMutation,
styleAttributeValue,
Optional,
} from '@rrweb/types';
import {
Expand Down Expand Up @@ -438,10 +437,29 @@ export default class MutationBuffer {
// text mutation's id was not in the mirror map means the target node has been removed
.filter((text) => this.mirror.has(text.id)),
attributes: this.attributes
.map((attribute) => ({
id: this.mirror.getId(attribute.node),
attributes: attribute.attributes,
}))
.map((attribute) => {
const { attributes } = attribute;
if (typeof attributes.style === 'string') {
const diffAsStr = JSON.stringify(attribute.styleDiff);
const unchangedAsStr = JSON.stringify(attribute._unchangedStyles);
// check if the style diff is actually shorter than the regular string based mutation
// (which was the whole point of #464 'compact style mutation').
if (diffAsStr.length < attributes.style.length) {
// also: CSSOM fails badly when var() is present on shorthand properties, so only proceed with
// the compact style mutation if these have all been accounted for
if (
(diffAsStr + unchangedAsStr).split('var(').length ===
attributes.style.split('var(').length
) {
attributes.style = attribute.styleDiff;
}
}
}
return {
id: this.mirror.getId(attribute.node),
attributes: attributes,
};
})
// attribute mutation's id was not in the mirror map means the target node has been removed
.filter((attribute) => this.mirror.has(attribute.id)),
removes: this.removes,
Expand Down Expand Up @@ -548,6 +566,8 @@ export default class MutationBuffer {
item = {
node: m.target,
attributes: {},
styleDiff: {},
_unchangedStyles: {},
};
this.attributes.push(item);
}
Expand All @@ -562,46 +582,43 @@ export default class MutationBuffer {
target.setAttribute('data-rr-is-password', 'true');
}

if (attributeName === 'style') {
const old = unattachedDoc.createElement('span');
if (m.oldValue) {
old.setAttribute('style', m.oldValue);
}
if (
item.attributes.style === undefined ||
item.attributes.style === null
) {
item.attributes.style = {};
}
const styleObj = item.attributes.style as styleAttributeValue;
for (const pname of Array.from(target.style)) {
const newValue = target.style.getPropertyValue(pname);
const newPriority = target.style.getPropertyPriority(pname);
if (
newValue !== old.style.getPropertyValue(pname) ||
newPriority !== old.style.getPropertyPriority(pname)
) {
if (newPriority === '') {
styleObj[pname] = newValue;
} else {
styleObj[pname] = [newValue, newPriority];
}
}
}
for (const pname of Array.from(old.style)) {
if (target.style.getPropertyValue(pname) === '') {
// "if not set, returns the empty string"
styleObj[pname] = false; // delete
}
}
} else if (!ignoreAttribute(target.tagName, attributeName, value)) {
if (!ignoreAttribute(target.tagName, attributeName, value)) {
// overwrite attribute if the mutations was triggered in same time
item.attributes[attributeName] = transformAttribute(
this.doc,
toLowerCase(target.tagName),
toLowerCase(attributeName),
value,
);
if (attributeName === 'style') {
const old = unattachedDoc.createElement('span');
if (m.oldValue) {
old.setAttribute('style', m.oldValue);
}
for (const pname of Array.from(target.style)) {
const newValue = target.style.getPropertyValue(pname);
const newPriority = target.style.getPropertyPriority(pname);
if (
newValue !== old.style.getPropertyValue(pname) ||
newPriority !== old.style.getPropertyPriority(pname)
) {
if (newPriority === '') {
item.styleDiff[pname] = newValue;
} else {
item.styleDiff[pname] = [newValue, newPriority];
}
} else {
// for checking
item._unchangedStyles[pname] = [newValue, newPriority];
}
}
for (const pname of Array.from(old.style)) {
if (target.style.getPropertyValue(pname) === '') {
// "if not set, returns the empty string"
item.styleDiff[pname] = false; // delete
}
}
}
}
break;
}
Expand Down
240 changes: 228 additions & 12 deletions packages/rrweb/test/__snapshots__/integration.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2933,24 +2933,14 @@ exports[`record integration tests can record node mutations 1`] = `
\\"id\\": 36,
\\"attributes\\": {
\\"id\\": \\"select2-drop\\",
\\"style\\": {
\\"left\\": \\"Npx\\",
\\"width\\": \\"Npx\\",
\\"top\\": \\"Npx\\",
\\"bottom\\": \\"auto\\",
\\"display\\": \\"block\\",
\\"position\\": false,
\\"visibility\\": false
},
\\"style\\": \\"left: Npx; width: Npx; top: Npx; bottom: auto; display: block;\\",
\\"class\\": \\"select2-drop select2-display-none select2-with-searchbox select2-drop-active\\"
}
},
{
\\"id\\": 70,
\\"attributes\\": {
\\"style\\": {
\\"display\\": false
}
\\"style\\": \\"\\"
}
},
{
Expand Down Expand Up @@ -3391,6 +3381,232 @@ exports[`record integration tests can record node mutations 1`] = `
]"
`;

exports[`record integration tests can record style changes compactly and preserve css var() functions 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\\": 2,
\\"tagName\\": \\"html\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 2,
\\"tagName\\": \\"head\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 3
},
{
\\"type\\": 2,
\\"tagName\\": \\"body\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 5
},
{
\\"type\\": 2,
\\"tagName\\": \\"script\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"SCRIPT_PLACEHOLDER\\",
\\"id\\": 7
}
],
\\"id\\": 6
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\\\n \\\\n\\\\n\\",
\\"id\\": 8
}
],
\\"id\\": 4
}
],
\\"id\\": 2
}
],
\\"compatMode\\": \\"BackCompat\\",
\\"id\\": 1
},
\\"initialOffset\\": {
\\"left\\": 0,
\\"top\\": 0
}
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": \\"background: var(--mystery)\\"
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": \\"background: var(--mystery); background-color: black\\"
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": \\"\\"
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": \\"display:block\\"
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": {
\\"color\\": \\"var(--mystery-color)\\"
}
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": \\"color:var(--mystery-color);display:block;margin:10px\\"
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": {
\\"margin-left\\": \\"Npx\\"
}
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 4,
\\"attributes\\": {
\\"style\\": {
\\"margin-top\\": \\"Npx\\",
\\"color\\": false
}
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
}
]"
`;

exports[`record integration tests can use maskInputOptions to configure which type of inputs should be masked 1`] = `
"[
{
Expand Down
Loading

0 comments on commit d872d28

Please sign in to comment.