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

okejminja · 2023-06-29T14:13:15Z

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

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 · 2023-06-29T14:13:17Z

⚠️ 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 · 2023-07-06T15:47:52Z

packages/rrweb/src/utils.ts

+  return value
+    .split(';')
+    .map((declaration) => declaration.split(':')[0].trim())
+    .filter((declaration) => !!declaration);


Does the same thing but is neater

Suggested change

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

.filter(Boolean);

Juice10 · 2023-07-06T15:52:07Z

packages/rrweb/src/utils.ts

+    return [];
+  }
+  return value
+    .split(';')


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

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 · 2023-07-15T22:52:59Z

packages/rrweb/src/record/mutation.ts

@@ -574,7 +575,10 @@ export default class MutationBuffer {
            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');


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

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 · 2023-07-15T22:57:49Z

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 · 2023-07-15T23:08:45Z

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

jotform/css.js#28

eoghanmurray · 2023-07-15T23:29:14Z

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 · 2023-07-16T15:32:30Z

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 · 2023-07-21T17:41:23Z

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 · 2023-07-22T12:24:44Z

@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 · 2023-07-24T19:45:56Z

(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.

…web-io#1246

eoghanmurray · 2023-07-27T15:11:07Z

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.

…web-io#1246

okejminja · 2023-07-28T14:00:24Z

Closing this PR since it got handled in #1268

eoghanmurray · 2023-07-31T14:43:02Z

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.

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

* Don't use the CSSOM when there's `var()` present as it fails badly rrweb-io#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>

eoghanmurray · 2023-10-10T11:29:02Z

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

okejminja added 2 commits June 29, 2023 10:55

Respect shorthand properties when generating attributes

bb604b5

Update snapshot & run tests

a7d7099

okejminja marked this pull request as ready for review June 29, 2023 14:18

okejminja added 2 commits June 29, 2023 16:19

Removed unused import

c47332f

Ran prettier

48fec9f

Juice10 reviewed Jul 6, 2023

View reviewed changes

Juice10 assigned eoghanmurray Jul 7, 2023

eoghanmurray reviewed Jul 15, 2023

View reviewed changes

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023

Don't use the CSSOM when there's var() present as it fails badly rr…

de1a95e

…web-io#1246

eoghanmurray mentioned this pull request Jul 27, 2023

Compact style mutation fixes and improvements #1268

Merged

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023

Don't use the CSSOM when there's var() present as it fails badly rr…

0472fcc

…web-io#1246

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023

Don't use the CSSOM when there's var() present as it fails badly rr…

6d58442

…web-io#1246

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023

Don't use the CSSOM when there's var() present as it fails badly rr…

2839f69

…web-io#1246

eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Jul 27, 2023

Don't use the CSSOM when there's var() present as it fails badly rr…

b25d447

…web-io#1246

okejminja closed this Jul 28, 2023

github-actions bot mentioned this pull request Aug 3, 2023

Version Packages (alpha) #1252

Merged

eoghanmurray mentioned this pull request Oct 10, 2023

Browser/Spec bug: Failing test for capturing css shorthand with confounding variable #1322

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

okejminja commented Jun 29, 2023 •

edited

Loading

changeset-bot bot commented Jun 29, 2023 •

edited

Loading

Juice10 Jul 6, 2023

Juice10 Jul 6, 2023

okejminja Jul 6, 2023

eoghanmurray Jul 15, 2023

okejminja Jul 21, 2023

eoghanmurray commented Jul 15, 2023

eoghanmurray commented Jul 15, 2023 •

edited

Loading

eoghanmurray commented Jul 15, 2023

eoghanmurray commented Jul 16, 2023

okejminja commented Jul 21, 2023

okejminja commented Jul 22, 2023

eoghanmurray commented Jul 24, 2023

eoghanmurray commented Jul 27, 2023

okejminja commented Jul 28, 2023

eoghanmurray commented Jul 31, 2023

eoghanmurray commented Oct 10, 2023

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

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

Conversation

okejminja commented Jun 29, 2023 • edited Loading

Description of the issue

Tests

changeset-bot bot commented Jun 29, 2023 • edited Loading

⚠️ No Changeset found

Juice10 Jul 6, 2023

Choose a reason for hiding this comment

Juice10 Jul 6, 2023

Choose a reason for hiding this comment

okejminja Jul 6, 2023

Choose a reason for hiding this comment

eoghanmurray Jul 15, 2023

Choose a reason for hiding this comment

okejminja Jul 21, 2023

Choose a reason for hiding this comment

eoghanmurray commented Jul 15, 2023

eoghanmurray commented Jul 15, 2023 • edited Loading

eoghanmurray commented Jul 15, 2023

eoghanmurray commented Jul 16, 2023

okejminja commented Jul 21, 2023

okejminja commented Jul 22, 2023

eoghanmurray commented Jul 24, 2023

eoghanmurray commented Jul 27, 2023

okejminja commented Jul 28, 2023

eoghanmurray commented Jul 31, 2023

eoghanmurray commented Oct 10, 2023

okejminja commented Jun 29, 2023 •

edited

Loading

changeset-bot bot commented Jun 29, 2023 •

edited

Loading

eoghanmurray commented Jul 15, 2023 •

edited

Loading