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.

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

fix(runner-ct): viewport scaling during screenshots #16543

Merged
merged 8 commits into from
May 20, 2021

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented May 17, 2021

User facing changelog

cy.screenshot was incorrectly capturing parts of the runner UI during screenshots.

Additional details

A lack of thorough testing around the cy.screenshot feature in CT runner led to some bugs and incorrect behavior. Runner CT now matches the behavior specified in the Cypress documentation and E2E runner, including:

  • respect the capture option to cy.screenshot correctly (both fullPage, which is the default, and viewport).
  • correctly remove scaling the AUT (the <iframe>) correctly during screenshot capture
  • now able to uncomment this test, since it works correctly:
    // TODO: This will technically pass, but the screenshot is not correct.

Several things contributed to the problems:

  1. scale applied the AUT during screenshotting.
  2. AUT needs to be scrollable (via scroll: overflow) during screenshot. That's why we had the problem in this ticket - the driver was trying to scroll to capture the entire viewport, but could not, since overflow: scroll was not set.

How has the user experience changed?

Figure 1: Regular 500x500 viewport

Notes: no change. The basic use case was fine.

Before:

image

After:

image

Figure 2: 750x750 viewport

Notes: depending on the size of the runner during the time of screenshot, this was okay. If you resize the browser window, though, certain sizings give you the gray background.

Before:

image

After:

image

Figure 3: component 1500px x 1000px, viewport 850x1000. cy.screenshot({ capture: 'viewport' })

This is how it looks in the runner UI:

image

Notes: "before" screenshot is completely wrong. It's capturing the entire page, not just the viewport, and there is a lot of the gray background showing. "after" is correct. Since we are specifying viewport, it is now correctly NOT capturing the hidden part (outside viewport).

Before:

image

After:

image

Figure 4: component 1500px x 1000px, viewport 850x1000. cy.screenshot({ capture: 'fullPage' })

Notes: it correctly scrolls and captures the entire page now.

Before:

image

After:

image

Figure 5: Regression from user project

See bug report here: #16478

When the browser is this size:

image

After:

image

Another bug was if I make the runner CT really small, like this:

image

Produced:

image

This isn't what the e2e runner was doing:

image

Now it's working correctly in CT:

image

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 17, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented May 17, 2021



Test summary

17945 0 214 7Flakiness 0


Run details

Project cypress
Status Passed
Commit aa9c1c3
Started May 20, 2021 12:53 AM
Ended May 20, 2021 1:06 AM
Duration 13:07 💡
OS Linux Debian - 10.8
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@lmiller1990 lmiller1990 marked this pull request as draft May 17, 2021 06:27

const iframe = document.querySelector<HTMLIFrameElement>('.aut-iframe')

iframe.classList.remove('aut-iframe-screenshotting')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty dirty, but unfortunately the <iframe> is not controlled by React. It's initialized here, and actually created here using jquery, so I don't any better way to do this. We need some specific styling to be applied during screenshotting to make everything look correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be OK temporarily, but the iframe code lives entirely inside runner-ct, so we can refactor it to be rendered via React.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice thing about this PR is we now have some Percy screenshots that are correct, so we can safely refactor.

@lmiller1990 lmiller1990 marked this pull request as ready for review May 18, 2021 02:58
Copy link
Contributor

@agg23 agg23 left a comment

Choose a reason for hiding this comment

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

Really nice to see these fixes. I have several questions about what this is doing.


const iframe = document.querySelector<HTMLIFrameElement>('.aut-iframe')

iframe.classList.remove('aut-iframe-screenshotting')
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be OK temporarily, but the iframe code lives entirely inside runner-ct, so we can refactor it to be rendered via React.

Comment on lines +35 to +39
.aut-iframe-screenshotting {
height: min(100vh, 100%) !important;
overflow: scroll !important;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What styles is this conflicting with? I particularly don't understand the min(100vh, 100%). Is the 100% the intrinsic content size, not the iframe parent's size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured this out just by creating lots of use cases and observing the output. Here's the use case this solves. Consider this example (note the viewport - 200x2000, kind of unrealistic use case, who has a device this shape? But still, good to test these things).

image

Without this CSS, the screenshot is like this. Note the default behvior of cy.screenshot, which is cy.screenshot({ capture: 'fullPage' }) (as opposed to cy.screenshot({ capture: 'viewport' }).

image

This is the desired output - scroll down the page, stitch together the screenshot:

image.

We need overflow: scroll to be applied. If overflow: scroll is not applied, Cypress tries to scroll, take a screenshot, and stitch them together. That's why you end up with that weird duplicating behavior in the first image above.

scroll: overflow is only applied if you have a static height set - height: 100% won't work. By doing min(100%, 100vh) it will apply height 100vh to components with a height greater than the viewport (100%). This supports the desired behavior.

I have some ideas on a much less hacky way to do this moving forward, I will send you a message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for this detailed explanation.

Comment on lines 48 to 49
transform: `scale(${screenshotting ? 1 : scale})`,
transformOrigin: `${screenshotting ? 'top left' : ''}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

transform: !screenshotting ? `scale(${scale}` : undefined

And I don't think you'll need transformOrigin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right - I didn't need transformOrigin. Nice catch.

Not sold on the transform refactor. Isn't an explicit scale(1) much more clear? Also, I think that if X then Y else Z is more clear than if not X then Z else Y syntax - ie, putting a negative at the start of the ternary feels a little weird, at least to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why emit HTML when you could not?

I suggested the ternary be written like that because I don't like having a falsy case be in the middle (the true case). Just a weird style preference of mine.

@@ -16,4 +16,5 @@
.size-container {
overflow: auto;
box-shadow: shadow(m);
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to limit against the viewportWidth? Can't this lead to situations where viewportWidth is larger than the window size, and the iframe will end up being smaller (or is viewportWidth the actual current browser size, in which case I'm not sure why this style is necessary)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. This might not be the right behavior. I added this to match the existing behavior in both the e2e runner and fix the regression reported by this comment.

I pulled his project and ran it. It looks like this in my browser.

Normal viewport:

image

Narrow viewport:

image

For reference, in CT runner it looks like this:

image

Without max-width: 100% we get the following screenshot. This just seems wrong (and a regression?). It should be centered, right? Or is this expected behavior? 🤔

image

With max-width: 100%:

image

E2E runner gives this:

image

So, I think this change makes sense - we should match what E2E does. You may notice that the header bar is missing in CT - I believe the user has some global CSS which he has not injected into the component, that is present when visiting the website. It's missing in both the CT runner and screenshot, so I think that's correct - the screenshot correctly reflects what is shown in the CT runner.

elevatebart
elevatebart previously approved these changes May 18, 2021
Copy link
Contributor

@elevatebart elevatebart left a comment

Choose a reason for hiding this comment

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

LGTM

@lmiller1990 lmiller1990 merged commit 25f59d1 into develop May 20, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 24, 2021

Released in 7.4.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v7.4.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants