[Found a fix] Problems with usage of puppeteer screenshooting

[r-oldenburg]

Hey guys,

I had several hard days of debugging when I found a solution to several randomly appearing problems that we encountered, whenever we switched to a Backstop (docker) version > 4.4.2.

And there are several other issues that (for me) look related to this...

Our problems:

• Screenshots were taken at the wrong location (having an offset left and top), sometimes with the wrong height
• Screenshots were different caused by transitions that should not have occured (i.e. hovered elements without any interaction or definition for it, elements moved b/c of an collapes/expand transition that normally should only happen on user interaction)
• This was happening completely unpredictable on any shot. But mostly happened on large viewport "lg" 1440x768 (we have 4 of them xs,sm,md,lg) and mostly on elements further down the page. Also more on the scenarios that were using selectorExpansion=true

Findings from investigation:

• It turned out that the height offset was somewhat related to the height of images on the page (offset sometimes was exactly a multiple of image sizes on the page)
• Any try with "waiting for the transitions/animations to end" did not help
• Also was it clear that we would have two types of problems:
• 1. Screenshots with wrong calculation of position/height
• 2. Screenshots taken while an animation/transition was ongoing

The Fix/Workaround, both within source of backstop:

• Do not call puppeteers screenshot function in parallel (currently happening via async call in core/util/runPuppet.js:391)
• Do pass in captureBeyondViewport: false

I will add a patch here.

Also did I have to change our viewport definition to height: 10000 (because captureBeyondViewport: false probably would keep us from getting screenshots further down on the page, I suppose. Haven't tested it without)

THAT FIXED ALL THE PROBLEMS FOR US.

For over a week now we are working with Backstop 5.3.4 in our CI pipeline (10 Devs, gitlab pipeline, ~10-20 MRs per day) using this patched version and it works just like a charm.

Interpretation:
I suppose there are two things happening here:

1. Puppeteer sometimes messes up screenshots when too many async calls come in in parallel (I could reproduce errors when I reverted my patch on line 391, with the patch they were gone)
2. Puppeteer triggers resize events (width 0, then target width) for the different viewports that in the following trigger several animations / transitions on all responsive elements (I could prove this by adding event listeners logging the events). This does not happen when passing captureBeyondViewport: false

[r-oldenburg]

Patch file cannot be attached, so I post it here:

From 5e28dd9030b4962245e938c39c133de32af1e592 Mon Sep 17 00:00:00 2001
From: Roland Oldenburg
Date: Wed, 21 Jul 2021 13:58:12 +0200
Subject: [PATCH] Workaround - cropped shots and resize-side-effects


diff --git a/core/util/runPuppet.js b/core/util/runPuppet.js
index 2bbb20f..61cc8c1 100644
--- a/core/util/runPuppet.js
+++ b/core/util/runPuppet.js
@@ -374,7 +374,7 @@ async function captureScreenshot (page, browser, selector, selectorMap, config,
           }
 
           var type = config.puppeteerOffscreenCaptureFix ? page : el;
-          var params = config.puppeteerOffscreenCaptureFix ? { path: path, clip: box } : { path: path };
+          var params = config.puppeteerOffscreenCaptureFix ? { captureBeyondViewport: false, path: path, clip: box } : { captureBeyondViewport: false, path: path };
 
           await type.screenshot(params);
         } else {
@@ -388,8 +388,8 @@ async function captureScreenshot (page, browser, selector, selectorMap, config,
     };
 
     const selectorsShot = async () => {
-      return Promise.all(
-        selectors.map(async selector => {
+        for (let i = 0; i < selectors.length; i++) {
+          var selector = selectors[i];
           filePath = selectorMap[selector].filePath;
           ensureDirectoryPath(filePath);
           try {
@@ -398,8 +398,7 @@ async function captureScreenshot (page, browser, selector, selectorMap, config,
             console.log(chalk.red(`Error capturing Element ${selector}`), e);
             return fs.copy(config.env.backstop + ERROR_SELECTOR_PATH, filePath);
           }
-        })
-      );
+      }
     };
     await selectorsShot();
   }
-- 
2.30.1 (Apple Git-130)

[garris]

@r-oldenburg I applied your patch and I am doing a release of this today. Would love to know if this fixes issues for others. Thanks much for this fix!

[r-oldenburg]

@garris Wow. That is a prompt reaction!
You maybe want to consider adding a switch to the config, allowing to pass in captureBeyondViewport. Or maybe even better add the possibility to pass custom params into the screenshot(...) call...?

[garris]

I am wondering if there is a way to do this automatically based on the type of screenshot the user is configuring? Maybe just for selectorExpansion=true type of sceenshots?

[r-oldenburg]

@garris
Maybe just for selectorExpansion=true type of sceenshots?

Possible, of course. But I am not sure if this really only happens in the selectorExpansion=true case.
And vice versa I am not sure if that setting doesn't break any other test constellation. Are you? Adding the possibility to change it from the outside would allow everybody to try out the fix and go back to the previous logic if it causes problems.

So: you could activate the new logic by default and allow users to go back.

[garris]

Crap -- I didn't think this would affect current implementations. I would rather that engineers can opt-in as opposed to opt-out.

[garris]

Removing the async part I am not that concerned about -- the captureBeyondViewport=false I guess could cause regressions in some cases.

[r-oldenburg]

captureBeyondViewport=false I guess could cause regressions if someone is trying to capture a whole document
Maybe, yes... Sorry, I should have mentioned my concerns from the beginning.

[garris]

Thanks @r-oldenburg -- I think the reason this would be an edge case is because whole document selectors are actually processed in a different flow earlier in the script. Someone needs to take a much closer look at this file and add comments -- right now -- it is totally unreadable. 🤦 There is a reason why this software is free! 😄

Anyway, If I can manage it I will look closer and add comments such that we could reason about what regressions we should expect.

Thanks for this fix though -- I think this catalysed some movement on this problem.

[r-oldenburg]

Thanks also for this great tool!!!

[r-oldenburg]

Still I would propose a switch or a config object to be optionally passed in. So that users can have a way out if they encounter regression.

[garris]

I agree we should have a switch. I will add this if I get a chance. You are more than welcome to post patch or PR with flag. Cheers!