Replace design screenshots with mShots in New Onboarding

[andrewserong]

Changes proposed in this Pull Request

• Generate an mShots screenshot for the design preview that passes in a locale to the template/demo endpoint
• Add an MshotsImage component that can deal with hiding the mShots default loading gif, and continue to request the image until it loads the final screenshot. The user should only see a pulsing rectangle until the image finally loads

Outstanding issues and todos

• The vertically oriented screenshots aren't quite how we'd like them, some of the viewport height based elements appear stretched vertically a little bit more than desired. This may or may not be a deal-breaker.
• Currently the image file size is a bit too much greater than the existing webp approach. Separately to this PR we're proposing adding in a quality param for mShots to support smaller file sizes.
• The logic to keep the image hidden until really loaded is a bit hacky and relies on a hard-coded URL. Is this safe enough, and does it hold up to testing when generating new screenshots? Is there a reliable way to test this, or write a test for it?
• Are we caching correctly? If a user goes back and forward between steps, does the browser re-download the images, or does it correctly cache them? We should ensure that the images aren't being re-downloaded multiple times.

Screenshot

Testing instructions

• Go to Calypso live URL below and append the mshots flag to the end of the URL, e.g. add ?flags=gutenboarding/mshot-preview or build locally and run calypso.localhost:3000/new/ko?flags=gutenboarding/mshot-preview.
• Either set your user language, or add a locale to the URL and go to Gutenboarding, e.g. /new/ko?flags=gutenboarding/mshot-preview or /new/de?flags=gutenboarding/mshot-preview
• At the design step, the previews should be translated
• Test this in a number of different languages
• Test this with limited Network speed (e.g. Fast 3G or Slow 3G in the Network tab in dev tools) — this will demonstrate that the file sizes for the images are currently too big.

Related to #37665
Fixes #48867

[matticbot]

Test live: https://calypso.live/?branch=update/gutenboarding-to-use-mshots-for-design-thumbnails

[andrewserong]

It seems to be working okay so far, specifying a large viewport height max, however designs that use viewport height in their layouts render strangely (or, well, accurately to their CSS and the viewport they're dealing with, but not how we want them to):

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~241 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding       +649 B  (+0.0%)     +241 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[simison]

specifying a large viewport height max, however designs that use viewport height in their layouts render strangely (or, well, accurately to their CSS and the viewport they're dealing with, but not how we want them to)

Yeah, Reynolds (and perhaps other designs?) need a bit extra CSS help somehow:

wp-calypso/bin/generate-gutenboarding-design-thumbnails.js

Lines 59 to 64 in 1227e50

	// Fix `reynolds_rockfield2_rockfield.jpg` first section becoming super tall
	// @TODO: fix at the source since this will be an issue with mshots API, too.
	const styles =
	design.slug === 'reynolds'
	? [ `.wp-block-cover, .wp-block-cover-image { min-height: ${ viewportHeight }px; }` ]
	: [];

[simison]

In case this is useful: https://github.com/Automattic/wp-calypso/blob/4537033c6794449ce0a10b69d72daf5fa658d553/client/lib/mshots/index.js

[andrewserong]

Thanks @simison that CSS does the trick. I've worked up a diff for adding the override to the template/demo endpoint, which should do the trick for mshots. (D45189-code)

[simison]

We could also add cache-busting timestamp coming from templates API?

[simison]

Might be better to have 700 in constant so that nobody misses updating it between previewUrl and mshotsRequest?

Edit: Oh, I just realized this was height and width, not height and height 😁

[simison]

if status is something else (e.g. an error 500), we could just keep looping otherwise for a while and then give up by disabling the preview thumbnail?

Not very high priority or blocking this PR though.

[andrewserong]

I'm wrapping up for the day and will be AFK until next week, when I'm then on gardening duty, so mightn't have time to work on this one for a couple of weeks. Feel free to pick this up, if anyone would like to!

Here are some notes / things to investigate from where I've gotten up to. While working on it I found it easiest to add "gutenboarding/mshot-preview": true to development.json for testing it locally.

Some to dos:

• The mShots endpoint supports CORS requests, however in Google Chrome, I notice that the first time it loads it succeeds, but if I browse forwards or backwards a step, then the network requests appear to load from cache, where the Origin header doesn't appear to be sent. So the response from the server doesn't have the access-control-allow-origin: http://calypso.localhost:3000 header, causing the images to fail to load. This will need to be investigated / resolved for this particular implementation to work in production. Update: Let's look into adding the crossorigin attribute to our link prefetches in prefetchDesignThumbs. This could be the culprit 🤞
• mShots URLs that 404 (e.g. the site that it's taking a screenshot of doesn't exist) results in a 404 image (with 200 response code) instead of a 404 response, so we can't really check for a 404 in our code and handle it. The 404 image is:

• Update the way the MshotsImage component fetches from the server, how many times do we want to check for a 307 before bailing?
• In the event of responses in the ranges 400 and 500, how do we want to handle what happens with the image?
• In testing locally, getting these images from mShots, even when they've already been generated, appears to be a bit slower to load than images stored in static. We should work out how to either a) make sure the images are loaded / cached earlier in the process, like we're currently doing for the static images when Gutenboarding loads, or assess whether we should still go with static images, but use mShots to generate them.

Just some thoughts, feel free to change / go in a different direction if you happen to pick up this PR.

Even though there's a fair bit to getting this working smoothly, the vertical screenshots are looking pretty good, and I'm happy so far with how the loading / onload state is looking:

[simison]

mShots URLs that 404 (e.g. the site that it's taking a screenshot of doesn't exist) results in a 404 image (with 200 response code) instead of a 404 response, so we can't really check for a 404 in our code and handle it. The 404 image is:

We could add a custom header in response at mshots?

[simison]

An alternative, just for your consideration:

const img = new Image();
img.addEventListener('load', (event) => {
 // event
}, false);
img.src = mShotsEndpointUrl;

Might help with cors stuff.

[andrewserong]

Oh, that's a nice idea! Unfortunately, I think we might still need to use the fetch approach if we want to hide the fallback /default route that renders the default loading gif, by handling the 307 redirect case.

It just occurred to me that I haven't looked into the link prefetch that we're doing in prefetchDesignThumbs — my hunch is that we need to add the crossorigin attribute, and this might explain why the CORS requests appear to be failing some of the time, but not when I directly reload the Design step. Just a hunch, though! I'll take a quick look early next week and see if it fixes it, if no-one beats me to it!

[deBhal]

however designs that use viewport height in their layouts render strangely

Yeah, it's looking great overall but there are a few little oddities. Different things in different previews, so I think in addition to the css fixes we'll want to pass size/viewport parameters through from available-designs-config.json to be able to make them all appropriately attractive.

[deBhal]

@simison @andrewserong : I was looking at Rockfield, and the viewport_height param on the template demo endpoint helps, but the images still end up the wrong size and looking pretty bad.

I had a play with mshots, and I think this might be a better solution than special-casing the css, but I wanted to make sure I wasn't just geeking out. Do you think this approach is worth completing? : Automattic/mShots#19

[andrewserong]

@deBhal nice, thanks for exploring this! It'd be worth commenting about it on p3topS-Ia-p2 as well, since there's some earlier related discussion there.

Personally I think that's a good direction to go down, considering that we'd like to be able to support a wider range of designs than we currently have, and who knows what other kinds of workarounds we might need to add for special-casing CSS in the future. If mShots can behave a little bit more like a "real" 16:9 browser that's scrolled down the page, then it might save us time further down the track to handle it in mShots. That said, I have very little knowledge of how mShots works, and it's used in a lot of other places, too, so we'd need to be careful that it doesn't conflict with how any other services are using it.

The other param I kinda wish we could support is a custom quality param. Right now it's locked to 90, but to reduce file-size when we're displaying a whole bunch of them, it'd be nice if we could pass in, say, 70 or something (or have a controlled set of low, mid, high quality settings?)

[ramonjd]

@deBhal Could you update the description and test plan so folks can start testing? Thanks!

[deBhal]

I haven't looked into the e2e failures, but we discussed in chat that they're not related to this PR, so assuming that's the case, I say .

There are a number of issues with this approach, but this PR is a clever & working solution to some of them.

My biggest concern is the 10 second delay between page load and starting the request that actually succeeds. It's hard to see mShots being a viable approach if it's going to be common for users to be waiting that long on rarer locales.

Again, though, this code is working impressively well, good job!