Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compact style mutation fixes and improvements #1268

Merged
merged 12 commits into from
Aug 3, 2023
Merged

Compact style mutation fixes and improvements #1268

merged 12 commits into from
Aug 3, 2023

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
Juice10
Juice10 reviewed Aug 3, 2023
View reviewed changes
Copy link
Contributor

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

packages/rrweb/src/record/mutation.ts Outdated Show resolved Hide resolved
packages/rrweb/test/__snapshots__/integration.test.ts.snap Show resolved Hide resolved
packages/rrweb/test/integration.test.ts Outdated Show resolved Hide resolved
packages/rrweb/test/integration.test.ts Outdated Show resolved Hide resolved
packages/rrweb/test/integration.test.ts Outdated Show resolved Hide resolved
packages/types/src/index.ts Outdated Show resolved Hide resolved
packages/rrweb/src/record/mutation.ts Outdated Show resolved Hide resolved
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
@eoghanmurray eoghanmurray merged commit d872d28 into rrweb-io:master Aug 3, 2023
6 checks passed
@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

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