Broken CSS in snapshots mixing longhand and shorthand CSS properties with CSS variables

luxaritas commented 1 year ago

Current behavior

On initial render, everything looks fine:

However when viewing the snapshot (which should be identical to the final state, as the test just loads the page), the background color is gone:

Desired behavior

No response

Test code to reproduce

<!DOCTYPE html>
<html lang="en">
<head>
    <style>
        .alert {
            --background: blue;
            background: var(--background);
            background-clip: border-box;
            width: 50px;
            height: 50px;
        }
    </style>
</head>
<body>
    <div class="alert"></div>
</body>
</html>

/// <reference types="cypress" />
describe('page', () => {
  it('works', () => {
    cy.visit('http://localhost:3000/')
  })
})

Cypress Version

12.4.0

Node version

v16.18.0

Operating System

Linux 6.1/Manjaro Linux 22.0.0

Debug Logs

No response

Other

I've identified where this issue manifests: https://github.com/cypress-io/cypress/blob/a869d5dd2890666220efea807e9a65f787d256bf/packages/driver/src/cy/snapshots_css.ts#L82-L90 (linking to inline CSS handling here, though it should ultimately be the same for linked stylesheets as well)

It appears when using a shorthand property (like background) along with one of it's components (like background-clip) and setting the shorthand to a CSS variable, when querying the stylesheet content in the DOM (via stylesheet.sheet.cssRules), the background property (in this case) gets split into its component properties, but these are not properly filled out - even though the browser renders it with the properties there (and they're shown in the devtools/inspector).

I'd imagine that the solution here is for inline stylesheets to use the innerHTML of the style tag and for external stylesheets to have the cypress server query them (due to cross-origin limitations)? The prior I could easily submit a PR for, though I don't know enough about the cypress codebase to know how to address the latter (and I'd want to confirm that this is the correct approach).

AtofStryker commented 1 year ago

Hi @luxaritas. Thank you for opening your first Cypress issue! I put together a reproduction repository that showcases the behavior you describe and I was able to reproduce the issue. I am going to route this over to a team.

luxaritas commented 1 year ago

Please let me know if it would be helpful for me to work on a PR for this if I can get a bit of guidance around the desired solution!

AtofStryker commented 1 year ago

@luxaritas it would be very helpful to work on a PR around this as I don't see this getting prioritized anytime soon unfortunately. What I think is going to be difficult is we store the body of the snapshot and not the whole document, which means the variables likely go away when rendered.

I would try to see if there is a way to persist those CSS variables in state with the snapshot or a way to clone the document instead of the body (which might be memory intensive) to preserve those variables to get started.

luxaritas commented 1 year ago

Per my original post, that doesn't appear to be the issue - the issue is snapshotting CSS is done by getting style sheet content via the DOM, and for some reason what browsers return is consistently broken vs the source stylesheet when using shorthand+longhand+css variables. Capturing variables doesn't help here, we need a way to get the actual source stylesheet content.

AtofStryker commented 1 year ago

oh I see sorry I missed that. Have you given the inline approach you outlined above? If you are able to get something working we can likely help test some of the limitations?

luxaritas commented 1 year ago

Not yet, but I certainly can a bit later. When I do, should I PR it immediately or just link back my fork here for discussion?

AtofStryker commented 1 year ago

Link back here for discussion should be fine and we can discuss 🙂 . Thank you for taking a deeper look into this!

luxaritas commented 1 year ago

Working on https://github.com/luxaritas/cypress/tree/fix/snapshot_var_mixed_longform, initial work with failing tests and the proposed fix for inline at https://github.com/cypress-io/cypress/commit/a198632c1cbd840211f03539c5279746ec6c01e9 I get the following two test failures with my proposed fix:

The second one appears to just differ on the presence of src: - I assume that test could just be modified to account for the lack of the internal browser transformation. The first one, however, is much more interesting - if you use sheet.insertRule, naturally that won't be propagated to the innerHTML of the style element. I'm not completely sure how to resolve that. Would it be valid to insert the original stylesheet innerHTML and then a "duplicate" with the last value provided by the DOM, such that if there were any changes they would be overridden, but if the original literal content incorrectly got "dropped", it would still be present?

Also it looks like cross-origin stylesheets are left as links. Looking at the history of that code, I don't clearly see why same-domain stylesheets weren't left as links as well - though it does run into the same issue with "what happens when it's modified via JS" - and again, maybe that could be resolved by including both the original link and a snapshot. if for some reason it had to be an actual request when same-origin, I'd need to know what the mechanism to make that request - and how to manage the possibility of it introducing async behavior when snapshotting appears to all be expected to be synchronous.

My proposal still wouldn't work if you did something like styleEl.sheet.insertRule('.foo {--background: orange;background: var(--background);background-clip: border-box;width: 50px;height: 50px;}', 0). In that case I don't see any way to get the correct, un-mangled CSS back out of that - I guess at that point It'd need to be fixed in Firefox and Chrome directly (just checked, WebKit doesn't have this issue)

luxaritas commented 1 year ago

For reference, the related bug already exists in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1218159&q=cssText&can=2 And Firerefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1341936 Apparently, this is actually an open limitation in the CSSOM spec with an issue pending action by w3c: https://github.com/w3c/csswg-drafts/issues/2515

AtofStryker commented 1 year ago

@luxaritas does styleEl.sheet.insertRule currently work with the CSS ruleset -> rules. Based on those bug reports, it doesn't look like works currently for the same reason? Based on your comment though you might be able to solve the first case? Just skimming through the commit without too much deep diving so I'm still trying to grok the understanding.

luxaritas commented 1 year ago

@AtofStryker If you're asking "does the current approach getting the parsed rules via the CSSOM provide the correct output for styleEl.sheet.insertRule('.foo {--background: orange;background: var(--background);background-clip: border-box;width: 50px;height: 50px;}', 0)", the answer is no.

Onurfesci commented 1 year ago

Is there any workaround to this at all?

I use Joy UI which inherently uses inline variables, and snapshots from Cypress tests have broken styles for this reason. So I either can't use inline variables at all, or I'm completely blocked from any visual regression testing. :(

cypress-io / cypress