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

cy.screenshot doesnt get the full screen #1857

Closed
egucciar opened this issue Jun 1, 2018 · 26 comments · Fixed by #1864
Closed

cy.screenshot doesnt get the full screen #1857

egucciar opened this issue Jun 1, 2018 · 26 comments · Fixed by #1864
Labels
pkg/server This is due to an issue in the packages/server directory type: bug
Milestone

Comments

@egucciar
Copy link
Contributor

egucciar commented Jun 1, 2018

Is this a Feature or Bug?

Bug

Current behavior:

here's an example screenshot taken by cypress:

image

Desired behavior:

This only captured a small section of the UI. the whole UI should be captured.

Steps to reproduce:

unsure of the specific steps. It works well when I ran on the big monitor, and fails when i run on the small laptop monitor.

Versions

MacOS and Cypress 3.0.1

@brian-mann
Copy link
Member

brian-mann commented Jun 1, 2018

What is the UI supposed to look like? It looks like it captured both the left and the right, but for whatever reason the form is being rendered to the right.

When we capture screenshots we "unscale" the app so its all full viewport resolution. If you've set your viewport to really high values and your screen is unable to display everything... it will end up looking like this.

We'll need more information about your viewport, the commands you're running, resolution / screen size, etc.

Also maybe a picture of what you "expect" it to look like would be nice.

@chrisbreiding

@egucciar
Copy link
Contributor Author

egucciar commented Jun 1, 2018

@brian-mann I can get you some more screens shortly.

When the app is unscaled and scrolled down I can see the full form. The form renders in the center of the app, with white space to the left and right. It also mises the entire bottom. I'll get back to you asap with a gif and more screens

@mike-hearn
Copy link

mike-hearn commented Jun 3, 2018

I’m seeing the same thing and I think it may be related to whether the screenshot is being taken on a regular DPI vs. high DPI screen. If I run it on a standard 1x screen it grabs the full viewport, but if I run it on a 2x screen (like a retina laptop), it grabs half of the viewport.

The ultimate screenshot dimensions are correct – for example, if the Cypress viewport is 1440x900, the generated image will be 1440x900 – but the captured page is doubled in size so the 1440x900 image only contains the top-left quadrant of the 2x page. Ideally either the final screenshot should be 2880x1800, or it could be resized down to 1440x900 for consistency across different environments.

@brian-mann
Copy link
Member

brian-mann commented Jun 3, 2018

@mike-hearn we did deal with a lot of DPI issues when developing the algorithms so I am sure that is the root cause. The day before 3.0.0 release I spent all day fixing a few bugs in the underlying subsystem.

The problem is that our e2e tests that go through this are using both a low res and high res screen with 2x DPI (and they all pass) - so this problem may be a more complicated problem than that. Can either of you create a simplified reproducible example?

As for resizing the images - we did consider this, as I agree that these things need to be "normalized" against a consistent environment. However, the need for this consistency extends beyond just the screen and get's into areas such as browser version, operating system version, whether fonts are installed or not, the host's screen resolution, etc, etc.

The ideal path of screenshot diffing is to use a homogenous environment - which means you pick a single environment that generates the masters and then only runs the screenshot diffing tests against that same environment. Even if we scale the images, there will always be differences unless things are 100% homogenous.

Re: scaling - there might be an algorithm out there for scaling down a 2x DPI image to a 1x DPI but it has to be lossless - else it will not be deterministic. I took a cursory glance and couldn't find anything off the shelf that did this - it all involved your typical image heuristics and various algorithms that would modify the actual image.

@brian-mann
Copy link
Member

@mike-hearn and @egucciar disregard what I previously said - I was able to reproduce this locally. We'll get this bug fixed.

@mike-hearn I played around with some things and think we may be able to resize 2x retina images to 1x losslessly but I'll have to play with it more. There are also 1.5DPI screens as well and I'm not sure those pixels can go to 1x.

Thinking about this more... even if we were able to get 2x or 3x down to 1x, it's not really truly "apples to apples". These days the modern web is capable of serving different resolutions to different screens. For instance, on a retina screen you may be getting served different content or entirely different images than say a non retina screen. Resizing those images may not yield deterministic results when you also try to diff on the 1x non retina screen.

Do you mind talking through the process you're trying to implement? You could also resize the images yourself after a cypress run if you wanted. I'm just trying to consider whether this should be the default behavior of Cypress.

@brian-mann
Copy link
Member

Here's an example of the bug...

Correct:
1x DPI (1000x1800)
app-screenshot

Incorrect:
2px DPI (1000x1800)
app-screenshot 2x

The 2x DPI image is cropped to be 1000x1800 when in fact it should be 2x that.

TODO: investigate how e2e tests are passing on retina tests and yet this is broken... 🤔

@bahmutov
Copy link
Contributor

bahmutov commented Jun 3, 2018 via email

@egucciar
Copy link
Contributor Author

egucciar commented Jun 3, 2018

@bahmutov I do have diffing implemented for the time being, but it's hard to figure out the edge cases with this bug right now.

As for the same environment diffing, I think that it should be possible to achieve reliable diffing results from one local environment to the next. For example, there are other programs which support visual diffing and are able to do this, however I'd like full integration in the Cypress ecosystem as it makes sense to diff durrung e2e tests and you guys are improving the screenshot capabilities as we speak. So I'm hoping with the new methodology - without scaling the UI - we could get those results but with the dpi bug cannot currently test the scenario

@brian-mann
Copy link
Member

The DPI bug will be fixed for sure. I just spent all night working on it and would like to get it out in the next patch release or two.

@brian-mann
Copy link
Member

brian-mann commented Jun 4, 2018

I have found the root cause of the bug.

BTW I tried some diffing against retina vs non retina images. The retina image was scaled back down to 1x DPI. It does indeed fail the diff because the text is rendered differently. Everything else was the same minus me changing my resolution to be 2x DPI vs 1x DPI. Note I set the threshold to 0 just so it would find every single different pixel.

My only point is that changing anything about the environment will result in differences in pixels. While you can downscale a 2x DPI image to a 1x DPI losslessly - it doesn't matter because it will never 100% match what the browser would have rendered if it was in a 1x DPI screen vs a 2x DPI screen. That's the difference.

pixelmatch ./cypress/screenshots/app-screenshot.png ./cypress/screenshots/app-screenshot-retina-scaled.png ./cypress/screenshots/diff.png 0
match: 139.222ms
different pixels: 25279
error: 1.4%

Source 1x DPI

app-screenshot

Source 2x DPI scaled down to 1x DPI

app-screenshot-retina-scaled

Diff

app-screenshot-diff

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

@brian-mann i was going to test with different thresholds and find the right threshold to detect regressions without flagging text across differing DPIs as false positives.

@brian-mann
Copy link
Member

@egucciar I tried with a different lib - resemble.js with the option to ignore anti aliasing and it reduced the difference in pixels down to .01% so it seems like its pretty solid.

At the end of the day Cypress itself won't and doesn't care about the diffing algorithm - we will just be part of the process that makes it easy to see, review, and "accept" the base changes after things run in CI. We'll also be the machinery that helps to take deterministic screenshots to help you generate the base masters as well as automatically retry when tests run so that you can use diffing as an assertion that'll work the same way all of the other assertions do.

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

At the end of the day Cypress itself won't and doesn't care about the diffing algorithm - we will just be part of the process that makes it easy to see, review, and "accept" the base changes after things run in CI. We'll also be the machinery that helps to take deterministic screenshots to help you generate the base masters as well as automatically retry when tests run so that you can use diffing as an assertion that'll work the same way all of the other assertions do.

This is exactly the direction we would love to see Cypress go in so I'm glad thats where you're headed!

@jennifer-shehane jennifer-shehane added this to the 3.0.2 milestone Jun 4, 2018
@jennifer-shehane jennifer-shehane added the pkg/server This is due to an issue in the packages/server directory label Jun 4, 2018
@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

@brian-mann one quick question, when you said you don't care about a diffing algorithm , does that mean cypress will not support or never support a cy.diff or automated diffing process integrated into this review/accept/update workflow you mentioned? Or is it all manual human verification based?

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

@brian-mann another issue which i could log seperately but just putting here for now:

Scrolling down to take screens doesnt sound ideal to me. In a word of fixed & sticky content, Cypress stiches together a screenshot which ultimately looks nothing like your precious application in reality. Perhaps, it should calculate window.height, and stretch the viewport to include all of the screen so that fixed content does not get re-stiched together upon the scrolling?

@brian-mann
Copy link
Member

You could just take element screenshots and turn off scrolling if you don't want it. Or just hide the sticky content or black it out. Lots of ways to solve this.

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

@brian-mann what if you want a full-height screenshot of your screen w/o blacking out the fixed content? i just am unsure how best to solve for this because ideally, it would just show me the full height view but i can understand this is challenging given the current way screenshotting is implemented.

Sidenote: If i open another tab in the Cypress browser under test while its matching screenshots, it will capture shots from my active tab, not my test (took me awhile to realize this). This wouldnt be a problem if links wouldnt automatically open in Cypress browser whenever i run with open

@brian-mann
Copy link
Member

brian-mann commented Jun 4, 2018

These things are just not possible. We have to use what automation API's the browser gives us. You can't make clicking a link magically open in another profile.

As for the full-height of the screenshot, it is possible to do this with native events and tapping into the debugger protocol, but no other browser has it, so it will/would only work for Chrome. When we implement native events we'll utilize that to do full height screenshots without having to scroll the page.

We did discuss doing this from the get-go but instead opted for a cross browser solution.

@brian-mann
Copy link
Member

You also don't have to black anything out, you can literally just remove the sticky elements before the screenshot and add them back after (if it matters).

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

These things are just not possible. We have to use what automation API's the browser gives us. You can't make clicking a link magically open in another profile.

What i mean is, when i click on a link i.e. in Slack, it will open in the Cypress controlled browser while my tests are running. I don't know why it does this, but it means that while my tests are running and screenshots are happening, i best not click on any links in Slack while im at it. Strange and undesirable behavior I am unsure how to work around besides being careful not to click on links in my desktop apps while im in the midst of screenshotting 😅

@brian-mann
Copy link
Member

brian-mann commented Jun 4, 2018

I think it has to do with which profile was opened last, but I swear we've run experiments and nobody can figure out why sometimes links open in Cypress and other times they open in your profile. It's all up to Chrome to handle it.

However, I think that if you used Canary or Chromium instead of Chrome as the Cypress browser then links would always open in the default browser (Chrome) instead of in Cypress.

You could also run your tests in Electron and also bypass this problem. Though I understand why you might not want to do that.

@egucciar
Copy link
Contributor Author

egucciar commented Jun 4, 2018

awesome suggestions! looking forward to the bugfix and other improvements in the pipeline , especially the fix that will speed this up a bit :) 👍

@jennifer-shehane
Copy link
Member

Yes, I researched what causes links to open in the Cypress controlled browser extensively about 2 years ago.

Links will open, when your 'default' browser is set to Chrome, within the Profile that was “most recently created”. At the time I researched, Cypress had no way possible to affect this behavior.

@masonmark
Copy link

masonmark commented Jun 13, 2018

There is a lot of tangential discussion branching off the main topic here, which is that cy.screenshot() only captures a portion of the screen on high-resolution (pixel-multiplied / "Retina") screens.

In my own testing with Cypress 3.0.1, it seems to capture the left half of a page. (All my screens are high-res, so I can't test without.)

@brian-mann does your comment above imply that this particular aspect of the bug will be fixed soon in a minor release? (Awesome, if so.)

I tested this using my own blog, which is a tall page, and with the viewport set to 1600x1600 in cypress.json. The CLI works, as mentioned above, so these two results show expected vs actual results (I paste links instead of the images themselves, because they are very tall):

The spec code was:

describe('Cypress screenshot function', () => {

  it('can take screenshots', ()=> {

    cy.visit('http://masonmark.com')
    cy.screenshot()
  });

});

@egucciar
Copy link
Contributor Author

@brian-mann do you have an ETA for the 3.0.2 release?

brian-mann added a commit that referenced this issue Jun 18, 2018
@jennifer-shehane jennifer-shehane added stage: pending release and removed stage: in progress stage: needs investigating Someone from Cypress needs to look at this stage: proposal 💡 No work has been done of this issue stage: needs review The PR code is done & tested, needs review stage: ready for work The issue is reproducible and in scope labels Jun 27, 2018
@brian-mann
Copy link
Member

Released in 3.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/server This is due to an issue in the packages/server directory type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants