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

Default screen height should be the documentHeight #318

Open
staylos92 opened this issue Nov 16, 2015 · 18 comments
Open

Default screen height should be the documentHeight #318

staylos92 opened this issue Nov 16, 2015 · 18 comments

Comments

@staylos92
Copy link
Contributor

The default document height for screenshots is set to 1500px, this causes issues with some pages which are longer than this. Current workaround is to set something like:

screen_widths:

  • 320x5000
  • 600x5000
  • 1024x5000

in order to get the full page in the screenshot. This obviously isn't ideal if we end up with a very long page.

@wildlyinaccurate
Copy link
Contributor

We could quite easily set the viewport height to the full document height, but I wonder what effect that would have when diffing images with different heights?

Feel free to experiment -- you should be able to do this by changing the following line in javascript/_getDimensions.js from

'viewportHeight': parseInt(dimensions[2] || 1500)

to

'viewportHeight': parseInt(dimensions[2] || document.body.offsetHeight)

@ChrisBAshton ChrisBAshton added this to the 2.9.0 milestone Nov 30, 2015
@ChrisBAshton ChrisBAshton removed this from the 2.9.0 milestone Nov 30, 2015
@ChrisBAshton
Copy link
Contributor

Hmm, this isn't an easy fix. These dimensions are passed to Phantom/Casper before it loads the page - so the height is unreliable. Not to mention that the height might change at runtime, or after you do something with the before_capture hook.

I think what you'll want to do is put this in your YAML config:

before_capture: javascript/resizeHeight.js

And make a resizeHeight.js file something along the lines of:

module.exports = function (phantom) {
    phantom.viewportSize.height = document.body.offsetHeight;
    phantom.open(phantom.page.url);
}

dsingleton pushed a commit to alphagov/government-frontend that referenced this issue Feb 1, 2016
Add Wraith so that regressions can be easily spotted. Runs against a
set of supported examples. Run while pointing government-frontend at
the dummy content store, and an instance of static.

Usage:
On master run: wraith history test/wraith/config.yaml
On local branch run: wraith latest test/wraith/config.yaml

Had to run `bundle update` to get a version of nokogiri that would work with
`wraith`.

Configuration generated with `wraith setup` and combined with our
existing wraith usage, [1]. With modern versions of wraith you don't
need a `snap.js` file, as wraith will take care of the `phantomjs`
intergration, however we still need to set some cookies to prevent
intermitent UI (like the survey, or cookie bar) from interfering
with the diff results.

[1] alphagov/design-principles#246

This config seems to work OK-ish, though starts having time out issues with
many combinations of screen_widths, dogpiling the rails app and getting
timeouts. I couldn't find an obvious way to limit this without forking
wraith, so for now i've used a smaller set of screen_widths.

I also had to specifcy explicit heights for each screen_width, this seems
to be new to Wraith 3.x. Previous versions would capture the full page as
a default, where as now it will default to 1500px unless you specificy
a height. This isn't ideal as most of the page on a small screen_size is
much further down than 1500px. For now i've set some heights that deal
with some common cases, but i'm not sure this is a good approach long term.

There are some related wraith notes on that here:
- bbc/wraith#318 (suggestion doesn't work)
- bbc/wraith#296
@dsingleton
Copy link

I've been running into the same issue, wanting to get the the full page height, while using wraith with history/latest. We've used wraith in the past and the output was full height. I'm not sure if thats a change in wraiths behaviour, or perhaps in an un-versioned dependency on phantomjs behaving differently?

Currently, i'm trying to put together a wraith config for a new app, using the latest version of wraith, and a modern config (which is 👍 better). The behaviour we're after is full page height capture and image diffs.

I tried some variations of the phantom.viewportSize.height suggestion, but unfortunately couldn't get it to alter the capture height - it also looks like crop_images would override that before the diff is generated anyway :/

It sounds like this isn't supported right now, and not straight forward to add? If I can find the time i'd be interested in looking into this in more detail, but i'd be good to get a steer first 😄

(edit: sorry about the commit reference spam, there was some excessive rebasing)

@Gribnif
Copy link

Gribnif commented Feb 12, 2016

I, too, consider this a huge failing of version 3.x, as capturing with an arbitrarily large height of, say, 10000px has a major performance impact. A job that used to take about 2-3 minutes with version 2.x now takes over 10 minutes to perform the same analysis.

Why was this changed from version 2? Can the old behavior be restored?

@ChrisBAshton
Copy link
Contributor

I hear you - it would definitely be great to fix this.

Reopening the issue by popular demand, will try to look at this more closely in the coming weeks. In the meantime, pull requests are welcome!

Wraith v3.1.1 has more resilient image cropping, so should cope with dynamic heights much better, so hopefully this issue can be fixed more easily now.

@ChrisBAshton ChrisBAshton reopened this Feb 29, 2016
ChrisBAshton added a commit that referenced this issue May 22, 2016
ChrisBAshton added a commit that referenced this issue May 22, 2016
andreilupu pushed a commit to andreilupu/wraith that referenced this issue Oct 18, 2016
When there is no height parameter set allow the full height instead of the 1500 default

For bbc#318
@mootari
Copy link

mootari commented Dec 9, 2016

From what I'm seeing in phantom.js there is no way to set the viewport dimensions via a before_capture script as they will always be overridden by the configured dimensions.

@ChrisBAshton What needs to be done to move #439 forward?

@VGBenjamin
Copy link

Hello,

I currently use the version 4.0.0 with phantomjs but I still have the same issue when I didn't specify a height in the screen_widths attribute it is still resized to 1500px.
The _getDimensions.js didn't exist in the project should I create it? What is his content?

Regards,

Benjamin V.

@rithipooh
Copy link

Hello,

My friend and colleague found a wonderful solution to this issue. All we had to do was reduce the default height in helper.js to 100 instead of 1500 px in the function getWidthAndHeight(), and comment the line page.clipRect in Phantom.js to prevent setting the height. The code then takes the rendered height of the page for the screenshot!!! Try out and let us know how it works out...

@MrSleeth
Copy link

If this helps, I seem to have Wraith producing full-screen screenshots using Casper and the following:

  • Set a small, arbitrary height under the screen_widths section of the capture file and then have a pre-capture script similar to the below.

module.exports = function (casper, ready) { // test interaction on the page casper.wait(5000, function() { casper.viewport(320, 765); // this seems to take a dynamic height? ready(); }); }

@maheshm
Copy link

maheshm commented Jun 21, 2017

@rithipooh That worked perfectly fine. Do we really have to change the _helper.js file? Did not understand why that is being done. page.clipRect basically did the magic.

@ssteigen
Copy link

ssteigen commented Jul 31, 2017

If you're using phantomjs you can get full-page screenshots by commenting out a few lines in lib/wraith/javascript/phantom.js

diff --git a/lib/wraith/javascript/phantom.js b/lib/wraith/javascript/phantom.js
index 78f1a89..9332b86 100644
--- a/lib/wraith/javascript/phantom.js
+++ b/lib/wraith/javascript/phantom.js
@@ -132,12 +132,12 @@ function resizeAndCaptureImage() {
 
 function takeScreenshot() {
   console.log('Snapping ' + url + ' at: ' + currentDimensions.viewportWidth + 'x' + currentDimensions.viewportHeight);
-  page.clipRect = {
-    top: 0,
-    left: 0,
-    height: currentDimensions.viewportHeight,
-    width: currentDimensions.viewportWidth
-  };
+  // page.clipRect = {
+  //   top: 0,
+  //   left: 0,
+  //   height: currentDimensions.viewportHeight,
+  //   width: currentDimensions.viewportWidth
+  // };
   page.render(image_name);
 }

I think this is the solution pointed out by @rithipooh in #318 (comment).

@duartegarin
Copy link

Hi guys,
I have the same issue.
The fix to phantomjs seems to work fine, can this be fixed in Wraith?
I call this from within CI and every time spin a new docker instance so it would be awesome if it comes correct on install.
Cheers,

@bernhardberger
Copy link

Is there any news on this?

@terescode
Copy link

All,

There is a teribble, awful, awful way to workaround this without having to modify the core code so that it addresses this comment from @duartegarin.

If it weren't for the immediate need to have this resolved, I would never do this but what you can do is copy the snap file phantom.js and it's helper class _helper.js into your project directory, make the change to phantom.js described in this comment from @ssteigen. Then use the old snap_file config property to point to your local copy of phantom.js.

Again, I caution this is a terrible solution as you won't pick up maintenance or enhancements to the core copies of these files, but it will get you a temporary solution that works around having to patch the core code.

@terescode
Copy link

Here is a PR that attempts to integrate the suggestions above.

#572

@agarpac
Copy link

agarpac commented Jan 11, 2019

I have tried to make the changes of #572 and I can not make the screenshots in full screen, using screen_widths:
-1280
-1280xauto

The ones suggested in this comment either.

Am I the only one that doesn't work?

@ammaarrrrr
Copy link

Hey. I'm stuck with this issue. Was this issue resolved?

@coolsoftwaretyler
Copy link

We're considering using Wraith on a project and would also be interested in a resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests