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

lmiller1990 · 2021-05-17T04:18:54Z

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:

cypress/packages/runner-ct/cypress/component/screenshot.spec.tsx

Line 57 in 8700ecf

// TODO: This will technically pass, but the screenshot is not correct.

Several things contributed to the problems:

scale applied the AUT during screenshotting.
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:

After:

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:

After:

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

This is how it looks in the runner UI:

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:

After:

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

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

Before:

After:

Figure 5: Regression from user project

See bug report here: #16478

When the browser is this size:

After:

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

Produced:

This isn't what the e2e runner was doing:

Now it's working correctly in CT:

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 · 2021-05-17T04:18:57Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-05-17T04:25:38Z

Test summary

17945 • 0 • 214 • 7 • 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 · 2021-05-18T02:57:33Z

packages/runner-ct/src/app/useScreenshotHandler.tsx

+
+    const iframe = document.querySelector<HTMLIFrameElement>('.aut-iframe')
+
+    iframe.classList.remove('aut-iframe-screenshotting')


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.

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

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

agg23

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

agg23 · 2021-05-18T11:38:00Z

packages/runner-ct/src/app/useScreenshotHandler.tsx

+
+    const iframe = document.querySelector<HTMLIFrameElement>('.aut-iframe')
+
+    iframe.classList.remove('aut-iframe-screenshotting')


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

agg23 · 2021-05-18T11:41:57Z

packages/runner-ct/src/app/RunnerCt.scss

+.aut-iframe-screenshotting {
+  height: min(100vh, 100%) !important;
+  overflow: scroll !important;
+}
+


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?

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

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

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

.

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.

Thank you for this detailed explanation.

agg23 · 2021-05-18T11:43:34Z

packages/runner-ct/src/iframe/iframes.jsx

+            transform: `scale(${screenshotting ? 1 : scale})`,
+            transformOrigin: `${screenshotting ? 'top left' : ''}`,


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

And I don't think you'll need transformOrigin

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.

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.

agg23 · 2021-05-18T11:46:20Z

packages/runner-ct/src/iframe/iframes.scss

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


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)?

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:

Narrow viewport:

For reference, in CT runner it looks like this:

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? 🤔

With max-width: 100%:

E2E runner gives this:

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

LGTM

cypress-bot · 2021-05-24T20:20:55Z

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.

lmiller1990 added the component testing label May 17, 2021

lmiller1990 mentioned this pull request May 17, 2021

runner-ct: Component test cy.screenshot has unneeded background #16478

Closed

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

fix: correctly scale viewport

0d7d37d

lmiller1990 force-pushed the fix/screenshots branch from 148d72f to 0d7d37d Compare May 17, 2021 08:43

lmiller1990 added 4 commits May 17, 2021 19:31

correct height

91d7393

consider max width

28c6f52

Merge branch 'develop' into fix/screenshots

66c00d2

update tests

2cc6730

lmiller1990 commented May 18, 2021

View reviewed changes

lmiller1990 requested review from agg23, JessicaSachs and elevatebart May 18, 2021 02:57

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

agg23 reviewed May 18, 2021

View reviewed changes

elevatebart previously approved these changes May 18, 2021

View reviewed changes

remove unneeded css

6e82556

lmiller1990 dismissed elevatebart’s stale review via 6e82556 May 19, 2021 02:09

agg23 approved these changes May 19, 2021

View reviewed changes

elevatebart and others added 2 commits May 19, 2021 09:23

Merge branch 'develop' into fix/screenshots

fa67cac

Merge branch 'develop' into fix/screenshots

aa9c1c3

lmiller1990 merged commit 25f59d1 into develop May 20, 2021

cypress-bot bot locked as resolved and limited conversation to collaborators May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lmiller1990 commented May 17, 2021 •

edited

cypress-bot bot commented May 17, 2021

cypress bot commented May 17, 2021 •

edited

lmiller1990 May 18, 2021

agg23 May 18, 2021

lmiller1990 May 18, 2021

lmiller1990 May 19, 2021 •

edited

agg23 left a comment

agg23 May 18, 2021

agg23 May 18, 2021

lmiller1990 May 19, 2021 •

edited

agg23 May 19, 2021

agg23 May 18, 2021

lmiller1990 May 19, 2021

agg23 May 19, 2021

agg23 May 18, 2021

lmiller1990 May 19, 2021 •

edited

elevatebart left a comment

cypress-bot bot commented May 24, 2021


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

		iframe.classList.remove('aut-iframe-screenshotting')

		transform: `scale(${screenshotting ? 1 : scale})`,
		transformOrigin: `${screenshotting ? 'top left' : ''}`,

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

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

Conversation

lmiller1990 commented May 17, 2021 • edited

User facing changelog

Additional details

How has the user experience changed?

Figure 1: Regular 500x500 viewport

Figure 2: 750x750 viewport

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

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

Figure 5: Regression from user project

PR Tasks

cypress-bot bot commented May 17, 2021

cypress bot commented May 17, 2021 • edited

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 May 19, 2021 • edited

Choose a reason for hiding this comment

agg23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 May 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 May 19, 2021 • edited

Choose a reason for hiding this comment

elevatebart left a comment

Choose a reason for hiding this comment

cypress-bot bot commented May 24, 2021

lmiller1990 commented May 17, 2021 •

edited

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

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

cypress bot commented May 17, 2021 •

edited

lmiller1990 May 19, 2021 •

edited

lmiller1990 May 19, 2021 •

edited

lmiller1990 May 19, 2021 •

edited