Skip to content

Compact style mutation fixes and improvements#1268

Merged
eoghanmurray merged 12 commits intorrweb-io:masterfrom
eoghanmurray:styleShorthandWithVars
Aug 3, 2023
Merged

Compact style mutation fixes and improvements#1268
eoghanmurray merged 12 commits intorrweb-io:masterfrom
eoghanmurray:styleShorthandWithVars

Conversation

@eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented Jul 27, 2023 •
edited
Loading

1): CSSOM was failing badly with shorthand + variables as reported in #1246 e.g. in

elem.style = "background: var(--my-bg);"

the resultant mutation had no record of the variable. This is a deficiency in the spec for CSSOM as evidenced here:
https://bugs.chromium.org/p/chromium/issues/detail?id=1218159#c_ts1631714802

In this case we can fallback to the regular method before style attributes were special cased in #464, and which is still supported in replay.

2): I realized we can switch between the #464 method and the regular attribute method for a given mutation depending on which is longer. This prevents us converting the following:

elem.style = "background:black;"

to the much more verbose:

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

Whilst still preserving those cases where the #464 method is more terse. We achieve this by executing both methods and comparing string lengths of the two methods.
Comparison of the two methods upon emission is also the way we verify that the variable functions are being preserved.

@changeset-bot
Copy link

changeset-bot bot commented Jul 27, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 671b321

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
@rrweb/types Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/web-extension Patch
rrvideo Patch

Not sure what this means? Click here to learn what changesets are.

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

@eoghanmurray eoghanmurray mentioned this pull request Jul 27, 2023
Closed
@eoghanmurray eoghanmurray force-pushed the styleShorthandWithVars branch 3 times, most recently from 9a65c18 to 5c674f4 Compare July 27, 2023 15:59
@eoghanmurray
Don't use the CSSOM when there's var() present as it fails badly rr…
b25d447 
…web-io#1246
@eoghanmurray
As the CSS Object Model expands out shorthand properties, do a check …
17ba96c 
…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
}
@eoghanmurray
Updates to remainder of tests based on refined compact style mutations
1d0bb07
@eoghanmurray eoghanmurray force-pushed the styleShorthandWithVars branch from 5c674f4 to 1d0bb07 Compare July 27, 2023 16:02
Juice10
Juice10 reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

Great stuff @eoghanmurray and @okeminja!

I had a few tweaks for clarity, and I've spotted a potential bug, but overall this PR looks great!

eoghanmurray and others added 6 commits August 3, 2023 11:16
@eoghanmurray @Juice10
Update packages/rrweb/src/record/mutation.ts
e1eda15 
Use neater object destructuring

Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
@eoghanmurray @Juice10
Apply suggestions from code review
9af639e 
prefer waitForRAF

Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
@eoghanmurray @Juice10
Update packages/rrweb/src/record/mutation.ts
6c1ca1a 
frontload comment explaining what's going on here in terms of the 'var(' string check

Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
@eoghanmurray @Juice10
Update packages/rrweb/test/integration.test.ts
f2f1627 
Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
@eoghanmurray
Improve comments. Improve code readability by renaming variables
7fb0a5f
@eoghanmurray @github-actions
Apply formatting changes
52a31ee
Juice10
Juice10 reviewed Aug 3, 2023
View reviewed changes
@eoghanmurray eoghanmurray merged commit d872d28 into rrweb-io:master Aug 3, 2023
@github-actions github-actions bot mentioned this pull request Aug 3, 2023
Merged
eoghanmurray added a commit to eoghanmurray/rrweb that referenced this pull request Aug 3, 2023
@eoghanmurray
Compact style mutation fixes and improvements (rrweb-io#1268)
c0f9f1a 
* 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>
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Aug 7, 2023
@eoghanmurray @billyvg
Compact style mutation fixes and improvements (rrweb-io#1268)
885ffcf 
* 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 added a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
@eoghanmurray
Compact style mutation fixes and improvements (rrweb-io#1268)
c7102ea 
* 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 added a commit to eoghanmurray/rrweb that referenced this pull request Aug 8, 2023
@eoghanmurray
Compact style mutation fixes and improvements (rrweb-io#1268)
9c6e728 
* 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
Copy link
Contributor Author

eoghanmurray commented Aug 17, 2023

This is the spec issue for reference, however this patch can still stand even if it's later fixed in browsers
w3c/csswg-drafts#2515

@eoghanmurray eoghanmurray mentioned this pull request Oct 10, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Juice10 Juice10 Juice10 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eoghanmurray @Juice10