Compact style mutation fixes and improvements (#1268)

* Don't use the CSSOM when there's `var()` present as it fails badly https://github.com/rrweb-io/rrweb/pull/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>

.changeset/clean-plants-play.md

@@ -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

packages/rrweb/src/record/mutation.ts

@@ -18,7 +18,6 @@ import type {
   attributeCursor,
   removedNodeMutation,
   addedNodeMutation,
-  styleAttributeValue,
   Optional,
 } from '@rrweb/types';
 import {
@@ -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,
@@ -548,6 +566,8 @@ export default class MutationBuffer {
           item = {
             node: m.target,
             attributes: {},
+            styleDiff: {},
+            _unchangedStyles: {},
           };
           this.attributes.push(item);
         }
@@ -562,39 +582,7 @@ 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,
@@ -602,6 +590,35 @@ export default class MutationBuffer {
             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;
       }

packages/rrweb/test/__snapshots__/integration.test.ts.snap

@@ -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\\": \\"\\"
           }
         },
         {
@@ -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`] = `
 "[
   {

packages/rrweb/test/integration.test.ts

@@ -209,6 +209,56 @@ describe('record integration tests', function (this: ISuite) {
     assertSnapshot(snapshots);
   });
 
+  it('can record style changes compactly and preserve css var() functions', async () => {
+    const page: puppeteer.Page = await browser.newPage();
+    await page.goto('about:blank');
+    await page.setContent(getHtml.call(this, 'blank.html'), {
+      waitUntil: 'networkidle0',
+    });
+
+    // goal here is to ensure var(--mystery) ends up in the mutations (CSSOM fails in this case)
+    await page.evaluate(
+      'document.body.setAttribute("style", "background: var(--mystery)")',
+    );
+    await waitForRAF(page);
+    // and in this change we can't use the shorter styleObj format either
+    await page.evaluate(
+      'document.body.setAttribute("style", "background: var(--mystery); background-color: black")',
+    );
+
+    // reset is always shorter to be recorded as a sting rather than a styleObj
+    await page.evaluate('document.body.setAttribute("style", "")');
+    await waitForRAF(page);
+
+    await page.evaluate('document.body.setAttribute("style", "display:block")');
+    await waitForRAF(page);
+    // following should be recorded as an update of `{ color: 'var(--mystery-color)' }` without needing to include the display
+    await page.evaluate(
+      'document.body.setAttribute("style", "color:var(--mystery-color);display:block")',
+    );
+    await waitForRAF(page);
+    // whereas this case, it's shorter to record the entire string than the longhands for margin
+    await page.evaluate(
+      'document.body.setAttribute("style", "color:var(--mystery-color);display:block;margin:10px")',
+    );
+    await waitForRAF(page);
+    // and in this case, it's shorter to record just the change to the longhand margin-left;
+    await page.evaluate(
+      'document.body.setAttribute("style", "color:var(--mystery-color);display:block;margin:10px 10px 10px 0px;")',
+    );
+    await waitForRAF(page);
+    // see what happens when we manipulate the style object directly (expecting a compact mutation with just these two changes)
+    await page.evaluate(
+      'document.body.style.marginTop = 0; document.body.style.color = null',
+    );
+    await waitForRAF(page);
+
+    const snapshots = (await page.evaluate(
+      'window.snapshots',
+    )) as eventWithTime[];
+    assertSnapshot(snapshots);
+  });
+
   it('can freeze mutations', async () => {
     const page: puppeteer.Page = await browser.newPage();
     await page.goto('about:blank');

packages/rrweb/test/record/__snapshots__/cross-origin-iframes.test.ts.snap

@@ -2625,10 +2625,7 @@ exports[`cross origin iframes form.html should map scroll events correctly 1`] =
         {
           \\"id\\": 9,
           \\"attributes\\": {
-            \\"style\\": {
-              \\"width\\": \\"Npx\\",
-              \\"height\\": \\"Npx\\"
-            }
+            \\"style\\": \\"width: Npx; height: Npx;\\"
           }
         }
       ],

packages/rrweb/test/utils.ts

@@ -133,23 +133,26 @@ function stringifySnapshots(snapshots: eventWithTime[]): string {
           s.data.source === IncrementalSource.Mutation
         ) {
           s.data.attributes.forEach((a) => {
-            if (
-              'style' in a.attributes &&
-              a.attributes.style &&
-              typeof a.attributes.style === 'object'
-            ) {
-              for (const [k, v] of Object.entries(a.attributes.style)) {
-                if (Array.isArray(v)) {
-                  if (coordinatesReg.test(k + ': ' + v[0])) {
-                    // TODO: could round the number here instead depending on what's coming out of various test envs
-                    a.attributes.style[k] = ['Npx', v[1]];
-                  }
-                } else if (typeof v === 'string') {
-                  if (coordinatesReg.test(k + ': ' + v)) {
-                    a.attributes.style[k] = 'Npx';
+            if ('style' in a.attributes && a.attributes.style) {
+              if (typeof a.attributes.style === 'object') {
+                for (const [k, v] of Object.entries(a.attributes.style)) {
+                  if (Array.isArray(v)) {
+                    if (coordinatesReg.test(k + ': ' + v[0])) {
+                      // TODO: could round the number here instead depending on what's coming out of various test envs
+                      a.attributes.style[k] = ['Npx', v[1]];
+                    }
+                  } else if (typeof v === 'string') {
+                    if (coordinatesReg.test(k + ': ' + v)) {
+                      a.attributes.style[k] = 'Npx';
+                    }
                   }
+                  coordinatesReg.lastIndex = 0; // wow, a real wart in ECMAScript
                 }
-                coordinatesReg.lastIndex = 0; // wow, a real wart in ECMAScript
+              } else if (coordinatesReg.test(a.attributes.style)) {
+                a.attributes.style = a.attributes.style.replace(
+                  coordinatesReg,
+                  '$1: Npx',
+                );
               }
             }
 

packages/types/src/index.ts

@@ -283,7 +283,7 @@ export type textMutation = {
   value: string | null;
 };
 
-export type styleAttributeValue = {
+export type styleOMValue = {
   [key: string]: styleValueWithPriority | string | false;
 };
 
@@ -292,13 +292,15 @@ export type styleValueWithPriority = [string, string];
 export type attributeCursor = {
   node: Node;
   attributes: {
-    [key: string]: string | styleAttributeValue | null;
+    [key: string]: string | styleOMValue | null;
   };
+  styleDiff: styleOMValue;
+  _unchangedStyles: styleOMValue;
 };
 export type attributeMutation = {
   id: number;
   attributes: {
-    [key: string]: string | styleAttributeValue | null;
+    [key: string]: string | styleOMValue | null;
   };
 };