Skip to content
This repository was archived by the owner on Dec 18, 2019. It is now read-only.

Conversation

@benurb
Copy link
Contributor

@benurb benurb commented Sep 9, 2015

Hello!

this PR scales down screenshots taken on high DPI displays (e.g. apple's retina) to their logical dimensions before processing them further. This fixes images cut in half/thirds on this type of display.

The other solution discussed in #52 (multiplying all pixel values with the physical/logical pixel ratio) is probably good, too, but harder to implement. This PR can be a first step as it at least prevents screenshots from being cut in half 😄

Best regards
Ben

@wbeard
Copy link

wbeard commented Sep 9, 2015

👍

@christian-bromann
Copy link
Contributor

@benurb thanks for the PR. Unfortunately this won't do the trick. devicePixelRatio is barely supported across platforms and won't work for native apps. We need to find a way that is as cross browser compatible as possible.

@elicwhite
Copy link
Contributor

@christian-bromann, which platforms don't support devicePixelRatio? I know iOS devices, Chrome, Firefox, and Safari have supported it for a long while. Not sure about IE. It seems like while this specific approach might not be right, using devicePixelRatio is, and if it isn't available we would just default it to 1 for no scaling.

@christian-bromann
Copy link
Contributor

@TheSavior it might be a good first approach but we need a fallback in case for native app testing where we can't use execute.

I first checked mdn which doesn't have any information about support so I thought it is a new thing but apparently it has pretty good support according to caniuse. Ok let's merge this then.

christian-bromann added a commit that referenced this pull request Sep 9, 2015
@christian-bromann christian-bromann merged commit f698c94 into webdriverio-boneyard:master Sep 9, 2015
@benurb
Copy link
Contributor Author

benurb commented Sep 10, 2015

Hey @christian-bromann
Thanks for merging 👍
There was one important info missing in the PR message. This fix is targeting high dpi displays on desktop platforms. I thought on mobile there are a few more problems according to #32.

Ben

@benurb benurb deleted the pr/fix-high-dpi branch September 14, 2015 12:35
@kierzniak
Copy link

This solution doesn't work for me. When I have width set to 1336px and execute webdrivercss on retina display screenshots are two times smaller 668px.

Another problem is calculations. Everything is callculated after resizing but when screenshot is resized units are not proportional. E.g I want to capture header which has 65px height

{
    name: 'header',
    elem: '.header--primary',
    x: 0,
    y: 0,
    height: 65,
    screenWidth: [1336]
}

screenshot base width is 1336px because of devicePixelRatio = 2 screenshot is resized to 668px. New height of header should be 32,5px but webdriver is getting 65px, which is wrong value.

When I comment this PR everyting seems to be working.

@benurb
Copy link
Contributor Author

benurb commented Dec 28, 2015

Hey @kierzniak

can you please supply a little more info about your setup (Browser, Operating System, Screen)?

I'm not able to reproduce your problem on my MacBook Pro 15" Retina (neither Phantom, nor Chrome). Also this PR should not change any image dimensions. It just fixes the problem that only a quarter of the screen is visible on retina (2:1 physical:virtual pixels) screenshots.

With the fix:
testwithwidthheightparameter 500px

Without the fix:
testwithwidthheightparameter 500px

Note that the image dimensions did not change. The config was:

{
  name: '_',
  x: 0,
  y: 0,
  width: 100,
  height: 100,
  screenWidth: [500]
}

Also the cropped element looks like on non-retina displays.
btw: Afaik the elem and x/y/width/height are exclusive to each other.

Regards,
Ben

@kierzniak
Copy link

@benurb
Ok, I know where problem is. Screenshot from firefox is size of the window even on retina displays. Screenshot using chromedriver is bigger twice on retina. I don't know exaclty which lib problem it is but I think webdrivercss should handle this. What do you think?

There is even issue on chromedriver bug tracker: https://bugs.chromium.org/p/chromedriver/issues/detail?id=752

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants