[PS AMP] Transparent images show the BBC blocks placeholder behind

[chris-hinds]

Describe the bug
A transparent image still hows the BBC blocks placeholder behind which means it can be seen through the image.

To Reproduce
Steps to reproduce the behaviour:

1. Go to https://www.bbc.co.uk/news/uk-55808266.amp
2. Scroll down to Doubts over UK's medium-term vaccine roll-out
3. See the transparent image below

Expected behaviour
Transparent images should not show the placeholder image behind it.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: [e.g. iOS]
• Browser [e.g. chrome, safari]
• Version [e.g. 22]

Smartphone (please complete the following information):

• Device: [e.g. iPhone6]
• OS: [e.g. iOS8.1]
• Browser [e.g. stock browser, safari]
• Version [e.g. 22]

Testing notes

• This bug fix is expected to need manual testing.

Additional context
Add any other context about the problem here.

[HarveyPeachey]

From investigating the issue, this bug affected both canonical and amp. I've created a PR for the canonical fix as this was simple. However the same fix can't be applied to amp, as the solution involved using onLoad which can't be used on amp-image.

To fix the amp side of this issue I have created two draft PR's, one which is quick and easy fix with a slight downside, and one that should be the correct way to do it, as it makes use of AMP's built in placeholder feature.

#8837 - Quick and easy - This adds a background colour to the amp-image. However this means the BBC placeholder won't show while the image is loading. And if the amp image times out on the url it's loading from, it most likely will just show the coloured background and not the placeholder background.

#8836 - More convoluted fix - The code for the draft PR has been done in Simorgh, however this should be implemented into psammead-image-placeholder, the readme for this component would need re-wording for the amp support. This makes use of the aforementioned native placeholder attribute that amp components can use. However the placeholder attribute that's being applied to the <div placeholder> is being stripped out on render. I think this could be Simorgh's doing somewhere in the codebase, as the amp docs state that you can use any element as a placeholder. This would show the placeholder while the amp-image is loading and also fallback to the placeholder if the image doesn't load. This will require a bit more investigation to get it working correctly.

[simonsinclair]

This is a good investigation, Harvey. #8836 is the solution I would personally like to see. It makes use of native AMP behaviour and I think it would be worth investigating further to see if it can be made workable within Simorgh.

[HarveyPeachey]

#8914 Has been reverted due to the solution causing page weight to quadruple on Live for AMP pages. We need another solution or re-think how the fix in the PR should work.

The main issue with the PR is that psammead-image-placeholder's AMP implementation produces a lot of html for every single image. This is mainly due to it using an inline SVG for the placeholder image which is a very big string.

One solution might be adding the SVG as a static asset. However that would still mean the DOM might be too large due to it still having to render multiple placeholder/fallback amp images.

Or we could go forward with the quick and hacky method outlined in this comment #8811 (comment)

[andrewscfc]

#8914 Has been reverted due to the solution causing page weight to quadruple on Live for AMP pages. We need another solution or re-think how the fix in the PR should work.

The main issue with the PR is that psammead-image-placeholder's AMP implementation produces a lot of html for every single image. This is mainly due to it using an inline SVG for the placeholder image which is a very big string.

One solution might be adding the SVG as a static asset. However that would still mean the DOM might be too large due to it still having to render multiple placeholder/fallback amp images.

Or we could go forward with the quick and hacky method outlined in this comment #8811 (comment)

One thing to try harvey to assess where to bother going down the route of making the svg a static asset:

• Revert the revert PR (you can create a draft), run simorgh and save the page content somewhere
• Switch to latest and save the same page content somewhere
• Do a find for the inline svg in the 'revert' page content and replace it with an empty string
• Compare the page weight between the two pages, this shows you how much the media queries add to the page discounting the svg

A small page weight increase may be acceptable to fix a bug, either that or we make a product decision to not support placeholders or go down your alternative route 🤷 ?

[aeroplanejane]

Caught up to discuss this with Emily A. @andrewscfc @mukama @gavinspence
We have a few choices

1. increase page weight to solve the problem
2. Replace BBC blocks images with a colour placeholder
3. Do not fix the issue

We've ruled out that we don't want to go down option 1
@aeroplanejane chatting to PS webcore team re 2 and / or 3
Blocked until decision is made.

[aeroplanejane]

We're going to go with option 2 and use the colour fill (we have to fix this as these transparent images are widely used across PS)
Please let us know what's required.

[ryanmccombe]

You may already have considered this, but jpgs cannot have transparency, and the vast majority of our images are jpgs. We could keep the placeholder on those, if preferred

[aeroplanejane]

@ryanmccombe I'm not sure if that was consider tbh. Would it be a conditional placeholder based on the image file type then? I'm not sure if that's possible in AMP but if so it might be:

• Given the image to load is a png.

• then display colour block placeholder to the user

• Given the image to load is NOT a .png

• then display BBC blocks placeholder to the user

[HarveyPeachey]

@aeroplanejane @ryanmccombe

I've created a PR which incorporates Ryan's suggestion. It makes a lot of sense to do it that way instead of forcing a colour fill on every image type. I've implemented it so that it applies the colour fill on any image that's not a .jpg or .jpeg. Mostly because .png and .gif support transparency but I'm pretty sure I've only ever seen jpg and png's used, so it shouldn't really matter.