Shorthand inline CSS properties not saved correctly in the styleObj

[okejminja]

Description of the issue

Noticed that inline CSS properties that were defined as a shorthand and use CSS variables don't get included in the array created by Array.from(target.style) and therefor they are not iterated through.
What gets included is their attempt to convert them to longhand properties, which wrongly assigns the value of a CSS declaration.

So, for example, if a element looks like this <div style="background: var(--bg-white)"/> the styleObj it creates has only the property background-color with an empty value (it should be background: var(--bg-white)).

Instead of taking the result of target.style which will NOT include shorthand properties if not specifically asked for with target.style.getPropertyValue(), I wrote a little helper function to split the style attribute and get just the declarations that are actually there, using them as an iterator and keeping the rest of the functionality the same.

This way, if a shorthand was defined it will be specifically requested with target.style.getPropertyValue() and it's value captured in the styleObj

Tests

3. Ensure the test suite passes or ask for help as to why tests are failing

There is one test that fails when running yarn test outside of packages/rrweb, but it was failing even before my changes so I think that's fine.

I'm having weird thing happen with test/integration.test.ts and can't figure out what's wrong with it. Sometimes it passes and sometimes it fails like this. I don't think my changes are affecting this, but thought it's better to ask you guys

[changeset-bot]

⚠️ No Changeset found

Latest commit: 48fec9f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Juice10]

Does the same thing but is neater

Suggested change

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

[Juice10]

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

[okejminja]

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!

[eoghanmurray]

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

[okejminja]

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

[eoghanmurray]

Great spot!

Myself and Justin found a few more examples where the browser really doesn't do the right thing in Array.from(target.style), so yeah the right thing to do is to manually parse target.getAttribute('style').

I think we also need to drop getPropertyValue (and getPropertyPriority) though; I'm not sure why that wasn't done in your code?

In terms of the parsing issues that @Juice10 pointed out, I had a look at https://github.com/vnmc/boreas/ as a way to parse CSS, which turned out to be very unwieldy but ultimately isn't a goer because of the large size; I'll see if there's a way to reduce that but until then I'll be looking out for another parser; regex likely won't cut it although having said that I haven't looked at the spec yet.

[eoghanmurray]

https://github.com/jotform/css.js fails a simple .someSelector { background:url("has a semicolon;should not break");}

jotform/css.js#28

[eoghanmurray]

Is there an easy way to import non typescript aware modules without having to rebuild them to target ESM?
https://www.npmjs.com/package/css

[eoghanmurray]

I think the solution is going to be 'no parse' and instead fallback to the full attribute text for the case when var() is present.
String style attributes are still supported at playback time, but it does need some detailed work to ensure that the transition from object to string and back again works.

[okejminja]

eoghanmurray Thanks for starting a conversation, I really appreciate it!

I think we also need to drop getPropertyValue (and getPropertyPriority) though; I'm not sure why that wasn't done in your code?

Can you please elaborate? What would you use instead of these two to get the value of a property?

I think the solution is going to be 'no parse' and instead fallback to the full attribute text for the case when var() is present.
String style attributes are still supported at playback time, but it does need some detailed work to ensure that the transition from object to string and back again works.

Do you mean just taking the raw string value of a style attribute and not converting in to object, using it plain as that?

[okejminja]

@eoghanmurray @Juice10 is this something you guys had in mind?
If var() is present just use the string of a style attribute?

if (attributeName === 'style') {
  const varCheckRegex = new RegExp('var(.+)', 'gm');
  if (m.oldValue && varCheckRegex.test(m.oldValue)) {
    item.attributes.style = m.oldValue;
    break;
  }

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

[eoghanmurray]

(was afk last week)

Moreso like

-        if (attributeName === 'style') {
+        if (attributeName === 'style' &&
+            target.getAttribute('style').indexOf('var(') === -1 &&

but there's more to it than that as it's possible to have multiple style mutations in the same 'batch', which are emitted as a single mutation.

I'm preparing a PR with what I mean, the main bit is to get an integration test.

Thanks for adding a test, that is very helpful. I'm going to try to prepare an integration test which should test for presence at the end of the right values, no matter whether the shorthand or longhand are used in the intermediary state. Expansion to longhand makes sense in terms of the CSSOM, and also in terms of the way we are doing compact changes, however it's a pity the spec didn't properly set out what to do with CSS variables, or that browsers haven't handled that without the current loss of information.

[eoghanmurray]

The #1268 PR is ready for testing now if you wish to check it out; it implements a fallback to remove the special casing of the style attribute in certain cases. The regular string way is still supported by replay.

[okejminja]

Closing this PR since it got handled in #1268

[eoghanmurray]

For the record, the approach you took with record.test.ts was just as valid as the one I took with integration.test.ts as both are pretty much looking at recorded events (either directly, or through a snapshot in the integration test).

The e2e/webgl.test.ts test is a better model that I should have used; it does record and replay to check the result.

[eoghanmurray]

Unfortunately this type of thing is not just a problem in the mutations, but also in the static stylesheet as evidenced in #1322