Fix: Do not complain when endY is bigger than document height

[mehdivk]

This PR fixes the bug that was reported in this issue https://github.com/zinserjan/wdio-visual-regression-service/issues/14

As I mentioned in that issue, if part of an element stays out of viewport (need scroll to be visible) this plugin throws an error. Unfortunately, there is no documentation in code so I couldn't find what's the initial intention for this check. Anyway, what I've done is to use this module directly against our project and with this check removed from the code, wdio-screenshot successfully takes expected screenshot of the element.

As side note, I think we can remove the similar check for endX as well. Unit tests are passing but integration tests are failing in my end when I run npm run test:local due to some pixel ratio.

[zinserjan]

Thanks for your PR.

Unfortunately, there is no documentation in code so I couldn't find what's the initial intention for this check.

My intention for this check was to find a test case that fails. Initially I thought that when endY is bigger than the documents height, then we have to use the documentHeight instead to get properly taken screenshots. But I wasn't sure if that 's the right way so I ended up with that error instead :smile:

As I can see you just removed that check. Are the screenshots really ok? I thought that this may record screenshots from areas that are not visible for user and this should never happen.

Regardless of the direction, it would be awesome & necessary to get a simple integration test for this.

[mehdivk]

@zinserjan glad to hear that. I totally agree with the integration test for this scenario. As I mentioned I tried to run integration tests using npm run test:local and it fails in image comparison part and as far as I can say it's because of pixel ratio difference between my workstation and the one that was used to generate the reference images.

I wonder what was the environment that you ran integration tests on it (in terms of pixel ratio).

[zinserjan]

I wonder what was the environment that you ran integration tests on it (in terms of pixel ratio).

Pixelratio 1. I created some of the screenshots in a VM with Ubuntu, some others with Windows (for the IE cases).

When you are testing with a Retina Macbok, tests will fail due to the size (twice as big).

[mehdivk]

@zinserjan I managed to run integration tests on Ububunu and all of them are passing. I also added two more integration tests for the new scenario and it works. Can you help on broken tests in Travis/Appveyor please? What's the process of generating reference IE images, I just copied the Linux one.

[zinserjan]

@mehdivk

Screenshots for IE are generated by IE ;-) But I'm not sure which version it was. Must be IE 9 or IE 10.

I just looked over the test page and I didn't found any reason in my browser why this should fail and tried the tests with the old code. And indeed tests passes with the old code cause element height is smaller than documents height.

Can you update the test case in a way that it will fail with the original code? I think it should be enough to expand height of the element.

[mehdivk]

@zinserjan let's park IE screenshots for now as it's not a big deal. I updated the example so it fails with the original code and passes with the change. The endY > documentHeight usually happens when you disable the main scroll by overflow: hidden and enable it again using overflow: scroll on an inner element on the page. This is a known approach when you want to have a header that remains on top of the screen during scroll and a footer that sticks to the bottom of the screen during scrolling. Updated example tries to replicate this issue with the minimum code, so it doesn't have header/footer but uses the same approach.

Digging into this problem, I noticed that we don't have the support of scrolling an element instead of the whole page. With the original fix, we can take the screenshot but the screenshot is broken because we move up the whole page instead of a specific element. I think, allowing the user to define an element as scroll target would fix this issue. I'm open to any suggestion to fix this problem.

[zinserjan]

I noticed that we don't have the support of scrolling an element instead of the whole page.

Yes, this is right and that's the real issue here. Just disabling the endY > documentHeight check doesn't solve this.

With the original fix, we can take the screenshot but the screenshot is broken

With broken you mean kind of incomplete, right? Only the visible part of the element is recorded and the rest of the screen is filled with the next element (below the captured/desired element)?

I think, allowing the user to define an element as scroll target would fix this issue.

I think it's more complicated. We have to think about two use cases:

• element screenshots
• document screenshots

element screenshots

This should be easiest case, cause we could detect if the element is scrollable or inside a scrollable container. I'm not sure right now if we have to check the second condition. If the element is scrollable, we can do it the same way like with the document and move the element with css into position, take screenshot, move again and so on until it's completely recorded.

document screenshots

That's the funny part, when we want to consider the following use case:

disable the main scroll by overflow: hidden and enable it again using overflow: scroll on an inner element on the page.

This is already complicated to handle but we can increase the complexity when you have multiple elements with that behavior (e.g. side by side with different heights). How does the final screenshot look like? I think everyone has a different opinion about that.

[mehdivk]

Yes, this is right and that's the real issue here. Just disabling the endY > documentHeight check doesn't solve this.

Indeed.

With broken you mean kind of incomplete, right? Only the visible part of the element is recorded and the rest of the screen is filled with the next element (below the captured/desired element)?

Yes, the visible part will be available in screenshot and rest will be yellow in our case (background color of the page is yellow). This is due to behavour of transform: transition

or inside a scrollable container. I'm not sure right now if we have to check the second condition. I can't think of any solution to detect if an element is within a scrollable container in a recursive way because an element may be within a container that is not scrollable but the container itself is within another container that is scrollable.

This should be easiest case, cause we could detect if the element is scrollable or inside a scrollable container. I'm not sure right now if we have to check the second condition. If the element is scrollable, we can do it the same way like with the document and move the element with css into position, take screenshot, move again and so on until it's completely recorded.

I propose to add an scroll option to browser.saveElementScreenshot(). If provided we use related DOM element that matches the selector for the purpose of scrolling otherwise we use html. This way we won't break existing clients of the module.

disable the main scroll by overflow: hidden and enable it again using overflow: scroll on an inner element on the page.

Well I don't think we need to mess with the elements. If consumer provides a selector as scroll target, we assume that the element is already scrollable. Am I missing something?

This is already complicated to handle but we can increase the complexity when you have multiple elements with that behavior (e.g. side by side with different heights). How does the final screenshot look like? I think everyone has a different opinion about that. TBH I'm not user of document screenshot as I belive that it's not a good pattern to take screenshot of whole document for the purpose of CSS regression tests but other devs might use it for some other reason.

We can keep this small and add support of custom scroll to the element

[mehdivk]

@zinserjan I added my proposed solution for the scroll to the PR.

[zinserjan]

I'm not convinced that this will work in all cases. I still think that we need the original virtualScroll on html to get the scroll container into position and move the content with virtualScroll again and take screenshots.

I'll try to create an example. But maybe I'm wrong cause CSS isn't my daily business :laughing:

[mehdivk]

I'm not convinced that this will work in all cases. I still think that we need the original virtualScroll on HTML to get the scroll container into position and move the content with virtualScroll again and take screenshots.

TBH I can't think of a case in which we need to move to scroll two scrollbars at the same time to take a screenshot of an element, neither this PR is going to fix all possible situations. I updated the new tests, so scroll target is different from the element under the test to show they don't need to be the same element. I would like to think of this PR as a fix and a feature

• Fix: Don't complain when endY > documentHeight, this is a legit case and can be reproduced using new tests

• Feature: Allow consumer of the API to customize scroll target per test optionally.

[mehdivk]

@zinserjan is there any intention to proceed with this PR?

[mehdivk]

@zinserjan Hi. Are you still interested in this PR? We need this feature for our project and if it's not going to be merged we have to find another solution for our problem.

Thanks

[zinserjan]

@mehdivk Sorry for the delay but the new year is very time consuming and I haven't found the time, yet.

We need this feature for our project

Which of the following do you mean exacly?

• Fix: Don't complain when endY > documentHeight, this is a legit case and can be reproduced using new tests

• Feature: Allow consumer of the API to customize scroll target per test optionally.

At the moment I would like to avoid introducing a new option that allows the user to define a custom scroll target until #21 and #48 are resolved. My preference is a automatic scroll element detection, so that the screenshot stitching algorithm remains as a black box for the user.

[cdaringe]

hi all. i just got x-reffed here after suffering the following error endY is out of range. i also see that it's been open a while. can i help in any way to get it movin?

[cdaringe]

ah, & i should have asked earlier... is there a temp workaround?

[JefroB]

Just chiming in to say that this still exists. I'm also seeing a similar error with endX.