-
Notifications
You must be signed in to change notification settings - Fork 607
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
Take screenshot without resizing page #820
Comments
I'm also experiencing this problem - a fix or workaround would be great! |
Unfortunately using the beta descrived in #785 doesn't seem to be fixing this. |
The problem is really due to the way Chrome tries to take fullscreen screenshots by resizing the browser window. Here's some code that works around the issue by taking multiple screenshots and stitching together. It also can give you borders around each image which I think may be helpful for determining if the site is fitting correctly to the device screensizes (one of the big issues we'd been having which made us look at some sort of auto testing).
Obviously this needs to somehow be inserted nicely into BackstopJS or more preferably Puppeteer, but I'd stress that both of these libs would only be working around a bug in Chrome - but given the Chrome team still haven't fixed the bug with 1px wide lines rendering incorrectly on non-retina screens, I hold little hope for this getting any attention. |
@JamieMcNaught If this is an issue with Chrome, then why doesn't Puppeteer have this problem and Chromy does? They're both using the Chromium dev tools API. |
@matthew-dean sorry about the slow response - not sure if github doesn't mail me or I just missed it. My backstop knowledge (and the distinctions between Puppeteer / Chromy) is very low but this issue seems to happen in Puppeteer - but it's very specific to certain websites, such as the NowUI site (https://demos.creative-tim.com/now-ui-kit/index.html) which will resize the layout based on the screen height to make the 'opening view' 100% of the browser viewport size. This is fine until you try and take a fullsixe screen shot in Chrome, at which point it seems to momentarily resize the browser window to the full height of the length of the page (which causes the page to re-layout and the screenshot to be way to long). So my issue is "solved" by a custom version of runPuppet.js which ensures that rather than doing a Chrome Fullsize Screenshot (which you can emulate in Chome itself by doing Developer Tools -> Toggle Device Toolbar (CTRL-SHIFT+M) -> "..." menu (top right) ->Capture Fullsize Screenshot - try it on the NowUI page) it instead takes individual screenshots all the way down the page and stiches them together. It's not ideal, but the result is something like the screen below - and sorry about the previous screenshots - should have resized them down. |
@JamieMcNaught Are you planning to do a PR of your changed code into Backstop? |
Just opened a PR inspired by @JamieMcNaught comment here: #1029 |
@garris Up |
I just brought this in locally. Will push to a beta tag. |
@garris Why did use a constant |
yes was wondering the same why not letting that as part of the config? |
What is the purpose for changing the value? It appears that this value needs to be small enough not to cause an overflow in puppeteer (i.e. works around the the root cause) but large enough that append operations are not called more than is necessary (i.e performance optimization). Am I understanding that correctly? If that is the case then it seems we can simplify the API and just treat as feature flag. What do you think? |
In my point of view I think that let the user choose allow him to reach maximum performance in his environment, contrariwise you'll need to add a min/max value. But maybe it's overkill and nobody will use it. |
Yes just performance optimization. Hard coding the value is probably the simplest here. As an alternative approach we could do: // `mergeImgHack` is typically a Boolean but user can
// specify a number (px) for performance optimization
const screenHeight = typeof config.mergeImgHack === 'number' ? config.mergeImgHack : 2000; Not the cleanest but the whole thing is a hack anyway... |
@sballesteros -- yes that is totally fine -- you can just... const screenHeight = typeof config.mergeImgHack === 'number' ? config.mergeImgHack : MERGE_IMG_SEGMENT_HEIGHT; |
@garris @sballesteros I've tested by manually changing it and I'm getting much better results that with the default 2000px height. |
@IgorMilosavljevic This is in-fact merged. (was done manually) Documented here...
This is published to @latest. Please update this thread if this works for you -- thanks! |
@garris I don't understand, where is documentation about it in canary branch ? |
@garris Thank you! That was quick, gj man. @Its-Alex
which will use the default viewport height of 2000px. Or if you want a specific height, eg.
Hope this helps, let me know if you need more info. |
@IgorMilosavljevic Thanks I will check, do you know if this is documented somewhere ? The fact that we can use a specific value ? |
@garris @IgorMilosavljevic |
This update is now described in the project-level READMe, if that answers your question. |
@Its-Alex BackstopJS/core/util/runPuppet.js Line 360 in a1b0b22
@Its-Alex @An-dy1 The
Reason I wrongly assumed it should be placed inside the |
@garris Tested on (using
Thank you once again, this project is an absolute gem |
@IgorMilosavljevic thank you for your help with this! |
The
If capture @JamieMcNaught could you improve |
The given "mergeImgHack": true" solution is not working for me. |
I'm having some trouble with the mergeImgHack. I really want this to work, because I'm trying to test a site that has an element with Enter mergeImgHack! I was so excited when I found this option!!! Alas, it doesn't seem to be working properly for me 🙁 When I set the selector to "document" in the scenario, and add I can increase the height of the viewport, and then more of the image is filled in, but then I start running into the distortion of the element whose height is 100vh 🙁 I tried using I've mucked about in runPuppet.js, and managed to get it to save out the individual screenshots before merging them. I can confirm that the original screenshots are all white below the viewport height, including the part of the first image that extended below the viewport height. Sooo... It sure seems like the problem lies with the screen capture part of the process, rather than the merging part. I'm on a MacBook Pro running MacOS 10.15.5, node version 12.18.2, and backstopJS version 5.0.1. My backstop.json file contains the following: {
"id": "backstop_default",
"mergeImgHack": 1000,
"viewports": [
{
"label": "phone",
"width": 320,
"height": 480
},
{
"label": "tablet",
"width": 1024,
"height": 768
}
],
"onBeforeScript": "puppet/onBefore.js",
"onReadyScript": "puppet/onReady.js",
"scenarios": [
{
"label": "Homepage",
"url": "http://techequity.test/?ao_nolazy=1",
"referenceUrl": "https://techequitycollaborative.test/?ao_nolazy=1",
"selectors": [
"document",
".testing-container"
],
"misMatchThreshold": 0.1,
"requireSameDimensions": true,
"viewports": [
{
"label": "Desktop",
"width": 1920,
"height": 1000
}
]
}
],
"paths": {
"bitmaps_reference": "backstop_data/bitmaps_reference",
"bitmaps_test": "backstop_data/bitmaps_test",
"engine_scripts": "backstop_data/engine_scripts",
"html_report": "backstop_data/html_report",
"ci_report": "backstop_data/ci_report"
},
"report": ["browser"],
"engine": "puppeteer",
"engineOptions": {
"args": ["--no-sandbox"]
},
"asyncCaptureLimit": 5,
"asyncCompareLimit": 50,
"debug": false,
"debugWindow": false
}
Does anyone have any ideas on how I can further troubleshoot this so as to give @garris as much info as possible? I really want this feature to work, and I know the best way to accomplish that is to provide a really helpful bug report, (or fix it myself and submit a PR!) but I'm running into the limits of my JS skills 😛 |
I'm also facing the same problem. Using |
@abu-sithik it helped me |
Thanks @zahorulko , installing the beta version of BackstopJS worked for me! Before that, I was having the same problem as @megclaypool - the images below my viewport height were blank/missing. @garris Will the changes that are in the beta version for mergeImgHack be merged in to master at some point? I'm concerned about using a beta version but this feature is so useful that I'm hoping it will be fully supported in future versions of BackstopJS. |
Hi @flitt1 -- apologies for the delay following up on this. I have not had a chance to really look into this issue. I need to review the changes that are in the beta branch -- what is the actual version number that fixes the problem for you? I am concerned that this problem is actually a few separate issues based on implementation details. So while this particular set of changes addresses your problem it may actually cause regressions for others. It may require a different config flag. |
Note to self: commits between beta branch and master... v4.3.3...master |
@garris Thanks, I'm using beta version 4.3.3. Other than latest, I haven't tried installing other versions, so if it would help for me to try a few different versions just let me know. |
@flitt1 So, the beta branch is actually behind master. It is an older branch. Here are all the changes that have been made between 4.3.3 and master. Someone needs to review this change list, v4.3.3...master (all the commits since 4.3.3) to find out what changes may have changed/broke the behavior you need. Please feel free to review this list for something that would impact the behavior -- at a quick glance I see a commit that looks like it could have something to do with it... #1196. |
@garris Thanks, I'll take a look. |
@garris It looks like it is the version of puppeteer that is causing the problem. The feature works perfectly until the switch from puppeteer 1.15.0 to 2.0.0 (724f16e).
Scratch that, looks like it doesn't work with puppeteer 5.5.0. I'll keep digging. |
Hi Guys, Unfortunately I also had the same problem as @megclaypool , so I tried the beta version for now, like @abu-sithik suggested. Works so far - thanks! However, I now have a problem with my selectors now. As soon as I try to select content outside this first upper area, I get the following error message: Error capturing element .footer Error: Node has 0 height. I would have tried several approaches: delay, readySelector, wait in my onReady-script, wait for element in onReady-script Anyone have any ideas how I could solve this problem? Sidenote: The content is loaded dynamically, but the document screenshot is complete, which is why I assume that these elements should also be there? Cheers, Edit: I am using backstop@beta (4.3.3) with puppeteer 1.15.0 - @flitt1 did this error occur in your tests as well? |
There is definitely an issue with A combination of |
Hi, Was this fix removed in this PR? Does it mean there is no way to work around this issue now? We see similar thing in our screenshot tests. We've got an animation when resizing window and it seems sometimes the screenshot is taken while this animation is being played - probably meaning that the windows is resized before taking screenshot. And in that case we are getting failed screenshot test. |
Is there any way to take a screenshot without resizing the page? Unfortunately the template (see here - https://demos.creative-tim.com/now-ui-kit/index.html) I use has some sections which use things like:
.landing-page .page-header {
height: 100vh;
position: relative;
}
which unfortunately means that the page-header element becomes huge when taking the screenshot and doesn't represent what it actually looks like on the device.
See the attached screenshots - the shorter two are taken with "Capture Screenshot", scroll, "Capture Screenshot" etc. The longer one is taken with "Capture full size screenshot" - which is I think what BackstepJS is doing. I would expect the longer one to look like the two shorted ones stitched together, but as you can see it's quite different.
Is there any way to get BackstepJS to screenshot without resizing the page (or whatever it is that it's doing).
Hope this makes sense? Many thanks.
Update: 2018-11-13 - I've replaced the screenshots with links (as the hugescreenshots were overwhelming). However in my comment today (see below) I've added a scaled down comparison of what backstop currently does and how our patch works.
Screen 1
https://user-images.githubusercontent.com/1828283/42628830-33ad21ce-85c9-11e8-8372-e885a5b07f1b.png
Screen 2 (just scrolled down from screen 1)
https://user-images.githubusercontent.com/1828283/42628841-3bee2fae-85c9-11e8-8a4d-f445cc72216b.png
Screenshot from BackstepJS - notice the page has re-layed out and now looks nothing like screens 1 and 2?
https://user-images.githubusercontent.com/1828283/42628843-3ef98356-85c9-11e8-82f3-1c5cc4c1a139.png
The text was updated successfully, but these errors were encountered: