Adding support for Android devices #79

jcneally · 2017-08-05T00:48:39Z

Background

wdio-screenshot currently supports iOS devices but it doesn't seem like a big jump to also start supporting Android as well. This adds image normalizing support for android devices as well.

Note: I was only able to test this on Android Simulator for 4.4 and 5.1 through SauceLabs and it was working fine for me. It's possible that other versions may not be supported, but limited is better than nothing 🤷‍♂️

Changes

Modified ScreenDimension#getScale to check for Android as well.
Modified ScreenshotStrategyManager to check isMobile instead of isIOS
Added a fixedHeights.js file to keep track of mobile devices' known fixed heights. If different versions of Android/iOS have different heights, this can be further granulated.
Modified normalizeScreenshot to check for mobile and take into account the type of device you are using.
Added test cases 😄 May need some help since the blank screenshots in the repo were a bit confusing
Updated .gitignore to ignore mac .DS_Store files

zinserjan · 2017-08-08T17:55:08Z

Awesome! Thanks for working on this.

wdio-screenshot currently supports iOS devices but it doesn't seem like a big jump to also start supporting Android as well. This adds image normalizing support for android devices as well.

Android devices are already supported ;) Android devices with chrome (which is default since 4.4) don't need any further changes in wdio-screenshot. That's why we need to make sure that this PR doesn't break them.

May need some help since the blank screenshots in the repo were a bit confusing

The blank or very yellow screenshots are just some simple screenshots to test the cropping. I used just a single background color to avoid any cropping mistakes due to antialiasing or other possible error sources. I'm not even sure if this could happen and just wanted to eliminate this kind of error...

zinserjan

Thanks for this and the tests! I reviewed your changes and added some notes.

zinserjan · 2017-08-08T17:28:29Z

src/utils/ScreenshotStrategyManager.js

-    const { isIOS } = browser;
-    if (isIOS) {
+    const { isMobile } = browser;
+    if (isMobile) {


Is this really necessary for android simulators or in other words, is merging enough? iOS uses this to remove the grey line between the stiched screenshots.

For real android devices with chrome this shouldn't be executed, merging ist enough.

I see, let me check just using the merge strategy on Android.

zinserjan · 2017-08-08T17:29:33Z

src/utils/mobile/fixedHeights.js

@@ -0,0 +1,10 @@
+export default {


Delete this file. Have a look at my other comment for the reason.

zinserjan · 2017-08-08T17:33:48Z

src/utils/normalizeScreenshot.js

-async function normalizeIOSScreenshot(browser, screenDimensions, base64Screenshot) {
-  const toolbarHeight = 44; // bottom toolbar has always a fixed height of 44px
-  const addressbarHeight = 44; // bottom toolbar has always a fixed height of 44px
+async function normalizeMobileScreenshot(browser, screenDimensions, base64Screenshot) {


Please create a seperate normalize function for Android. Using a normalizer that does iOS and android at the same time is just complicated and uncecessary.
I made this mistake around two years ago within an unpublished webdrivercss fork with support for iOS and Android. In the beginning it looks like a good idea but it becomes unmaintainable very quickly...

zinserjan · 2017-08-08T17:37:32Z

src/utils/normalizeScreenshot.js

-  const addressbarHeight = 44; // bottom toolbar has always a fixed height of 44px
+async function normalizeMobileScreenshot(browser, screenDimensions, base64Screenshot) {
+  const mobileFixedHeights = (browser.isAndroid) ? fixedHeights.android : fixedHeights.iOS;
+  const toolbarHeight = mobileFixedHeights.toolbarHeight;


Does Android really have a toolbar at the bottom? I can't see any at your screenshots.
When not please remove this.

I need to update the test case screenshots.
I don't know why it appears in some cases either, but when I run locally 5.1 it doesn't appear. SauceLabs emulator for 5.1 android however, shows the following. Note the footer.

Uiii, this doesn't look like a case that is fixable. Especially with the offsets around the device screenshot and the android controls on the right side 😁

Ah, this is a screenshot from the saucelabs video 😅
The device screenshot is just the header, page, and footer. But that will need cropping

zinserjan · 2017-08-08T17:40:01Z

src/utils/normalizeScreenshot.js

+async function normalizeMobileScreenshot(browser, screenDimensions, base64Screenshot) {
+  const mobileFixedHeights = (browser.isAndroid) ? fixedHeights.android : fixedHeights.iOS;
+  const toolbarHeight = mobileFixedHeights.toolbarHeight;
+  const addressbarHeight = mobileFixedHeights.addressbarHeight;


I think the height of the adressbar is something that we can calculate with screenHeight - innerHeight = 77.

In my opinion this makes all static height values unnecessary, am I wrong?

The issue is that toolbar appears only in some cases. Is there a way to get the y offset of the viewport from the top of the screen? If so then I can calculate whether the footer appears. I think if I use screenHeight - innerHeight and the footer is there, it will be the height of the address bar + the toolbar.

zinserjan · 2017-08-08T17:41:00Z

src/utils/normalizeScreenshot.js

-  if (browser.isMobile && browser.isIOS) {
-    normalizedScreenshot = await normalizeIOSScreenshot(browser, screenDimensions, normalizedScreenshot);
+  // check if we have to crop navigation- & toolbar for iOS or Android
+  if (browser.isMobile) {


As already said, please separate iOS from android here for maintenance reasons.

zinserjan · 2017-08-08T17:42:16Z

src/utils/normalizeScreenshot.js

-  if (browser.isMobile && browser.isIOS) {
-    normalizedScreenshot = await normalizeIOSScreenshot(browser, screenDimensions, normalizedScreenshot);
+  // check if we have to crop navigation- & toolbar for iOS or Android
+  if (browser.isMobile) {


You need to make sure that you doesn't break real android devices with chrome here. They don't need additional cropping as the screenshots are the same like the desktop chrome.

Hmmm, does the viewportHeight and screenHeight return different values for real device chrome? If not then it should just pass through without cropping. If so then maybe there is a way to check for emulators.

zinserjan · 2017-08-08T17:43:50Z

.gitignore

@@ -9,3 +9,6 @@ yarn.lock
 # project
 .tmp
 lib
+
+# Mac OSX
+.DS_Store


OS dependent stuff should be ignored in a global .gitignore file, but I'm fine with merging this in to avoid this in the future.

I'm not sure what you mean by a global .gitignore, I presumed that was this.

zinserjan · 2017-08-08T17:53:54Z

src/utils/ScreenDimension.js

@@ -83,7 +83,7 @@ export default class ScreenDimensions {
  }

  getScale() {
-    if (this.isIOS) {
+    if (this.isIOS || this.isAndroid) {


Is there a test case for this change?

Let me see if I can add some off of the existing ones

jcneally · 2017-08-08T20:24:22Z

Thanks for following up! Wasn't sure if I would get a response 😄

Android devices are already supported ;) Android devices with chrome (which is default since 4.4) don't need any further changes in wdio-screenshot. That's why we need to make sure that this PR doesn't break them.

Hmm, what kind of android support exists already? I do testing through Saucelabs Simulator/Emulator, and this didn't work out of the box. The header on android gets stitched onto each screenshot and looks quite a mess. Perhaps this isn't an issue for real devices? But happens on emulator and it's unusable.

zinserjan · 2017-08-08T20:49:43Z

Hmm, what kind of android support exists already?

Every android with chrome should work in theory. We tried some Nexus & Samsung devices.

But I think that simulators never have chrome, just the old android browser. Give the real devices from saucelabs a try ;)

Adding support for Android devices

6334983

zinserjan requested changes Aug 8, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for Android devices #79

Adding support for Android devices #79

jcneally commented Aug 5, 2017

zinserjan commented Aug 8, 2017

zinserjan left a comment

zinserjan Aug 8, 2017 •

edited

Loading

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

zinserjan Aug 8, 2017

jcneally Aug 8, 2017

jcneally commented Aug 8, 2017

zinserjan commented Aug 8, 2017

Adding support for Android devices #79

Are you sure you want to change the base?

Adding support for Android devices #79

Conversation

jcneally commented Aug 5, 2017

Background

Changes

zinserjan commented Aug 8, 2017

zinserjan left a comment

Choose a reason for hiding this comment

zinserjan Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcneally commented Aug 8, 2017

zinserjan commented Aug 8, 2017

zinserjan Aug 8, 2017 •

edited

Loading