WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575

LilyL0u · 2024-04-29T15:08:48Z

Resolves JIRA [https://jira.dev.bbc.co.uk/browse/WSTEAMA-652]

All images are now webp, except for when webp is unavailable it is removed by the service worker.

After discussion with @eagerterrier the decision was made to leave the picture tag because we need the home and front pages to work with fallbacks because the images can't be intercepted by the service worker on these pages. We can get rid of the fallback logic and sw logic when the iOS versions fall out of use in 6-12 months.

Do we also need to make changes to https://github.com/bbc/simorgh/blob/0ef5df7d08bb6238832c7ff76514a1214d4a057a/src/app/legacy/components/Promo/image.jsx ?

Testing

List the steps used to test this PR.

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

…-images-in-simorgh

…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh

…-images-in-simorgh

alex-magana · 2024-04-30T13:34:05Z

public/sw.js

    if (req.headers.has('accept')) {
      supportsWebp = req.headers.get('accept').includes('webp');
    }


Consider using the supportsWebp inference below, since the comparison on the right hand side will set
supportsWebp to true or false without the need to set a default value via

let supportsWebp = false;

Suggested change

if (req.headers.has('accept')) {

supportsWebp = req.headers.get('accept').includes('webp');

}

const supportsWebp = req.headers.has('accept') && req.headers.get('accept').includes('webp');

Is this the logic that determines whether the OS supports webp? (Or do we need to lookup the User-Agent?)

Also, ideally we want to default supportsWebp to true

Yes, the comment already in the code when I started said '// Inspect the accept header for WebP support' above this bit of code. You might have written it a while ago actually haha! I am trying to check the headers on the versions of iOS that don't work, but SauceLabs is being very slow to start the dev tools

Cool thanks - yes it has been a while since I looked at this code, so memory is foggy!

To make life a bit easier for you, why don't you use the preview environment for Saucelabs - the preview pipeline is quite clever in that it will kick off a new release when you push commits to the PR. You can continue working on fixing the tests, but testing the behaviour on an environment which is more representative of a test environment (with service worker working properly)

src/app/lib/utilities/ichefURL/index.js

karinathomasbbc · 2024-05-01T11:19:46Z

public/sw.js

    if (req.headers.has('accept')) {
      supportsWebp = req.headers.get('accept').includes('webp');
    }


Is this the logic that determines whether the OS supports webp? (Or do we need to lookup the User-Agent?)

Also, ideally we want to default supportsWebp to true

public/sw.js

karinathomasbbc · 2024-05-01T11:23:38Z

src/app/components/Image/index.tsx

+    // I don't know if we need this????
+    // if (!hasFallback) return srcSet;
+    // if (pageType !== FRONT_PAGE && pageType !== HOME_PAGE)
+    //   return fallbackSrcSet;
+    // return undefined;


Shall we just leave this in to be on the safe side?

Don't we want to remove this as we want to return a webp srcset by default now?

simorgh/src/app/components/Image/README.md

Line 25 in 274e392

| fallbackSrcSet? | string | the fallback srcset (probably the jpeg images) |

The fallback should only be used when the browser doesn't support Webp

If we only know if the browser doesn't support webp by checking the header using the service worker, does that mean the fallback logic needs to be in the service worker? Or I need to check headers in this file too?

This bit:

'
const supportsWebp =
req.headers.has('accept') && req.headers.get('accept').includes('webp');'

On homepages and frontpages we still use the picture element as per this PR. The picture element will be ignored on old browsers that don't support webp and a jpg request will be sent from the fallback img tag. This consequently will mean the correct srcset should be used on old browsers and the fallback srcset is included as the srcset attribute on the fallback img tag.

Now for other pages we are moving to just using the img tag with the webp srcset as I understand it, the browser will choose an image from the webp srcset the service worker will intercept the request for the chosen image and remove the webp extension where necessary.

~~So in this respect the service worker does not need to be aware on srcsets~~
unless I've missed something?

I think I've missed something another comment incoming!

ref

srcset defines the set of images we will allow the browser to choose between, and what size each image is. Each set of image information is separated from the previous one by a comma

https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images#resolution_switching_different_sizes

A discussion with @eagerterrier led to thinking we should leave the picture tag until the problematic iOS version isn't used anymore. This would be 6-12 months. So I haven't removed it in this work.

'The picture element will be ignored on old browsers that don't support webp and a jpg request will be sent from the fallback img tag.'
Does this happen automatically? I don't need to add code to detect the browser not supporting webp?

Note for reviewers had a DM with @LilyL0u and further changes are needed to only render the picture element on home and front pages, this comment assumes this change is being made

I think we do need to retain this logic as the img tag should be the fallback of last resort for old browsers; with this in mind the img tag should always use the fallback srcset for home and frontpages that use the picture element. Older browsers that don't support the picture element are also unlikely to support service workers and webp so we are better providing a fallback.

On other page types the img tag can be used with the primary srcset with the service worker intercepting the image request and stripping the webp extension as appropriate.

karinathomasbbc · 2024-05-01T11:24:26Z

src/app/components/Image/index.tsx

-              </>
-            )}
+            {hasFallback &&
+              (pageType === FRONT_PAGE || pageType === HOME_PAGE) && (


Let's make this a wee bit easier to read

Suggested change

(pageType === FRONT_PAGE || pageType === HOME_PAGE) && (

([FRONT_PAGE, HOME_PAGE].includes(pageType) && (

src/app/legacy/components/Promo/image.jsx

karinathomasbbc · 2024-05-01T11:25:45Z

src/app/legacy/psammead/psammead-image/src/index.jsx

@@ -50,7 +50,7 @@ export const Img = props => {

  return (
    <>
-      {pageType === FRONT_PAGE && (
+      {(pageType === FRONT_PAGE || pageType === HOME_PAGE)(


Suggested change

{(pageType === FRONT_PAGE || pageType === HOME_PAGE)(

{([FRONT_PAGE, HOME_PAGE].includes(pageType))(

karinathomasbbc · 2024-05-01T11:26:08Z

src/app/legacy/psammead/psammead-image/src/index.jsx

      )}
-      {pageType !== FRONT_PAGE && (
+      {pageType !== FRONT_PAGE && pageType !== HOME_PAGE && (


Suggested change

{pageType !== FRONT_PAGE && pageType !== HOME_PAGE && (

{![FRONT_PAGE, HOME_PAGE].includes(pageType) && (

Originally tried this and typescript had a tantrum. Will try again as your syntax may be slightly different, let's see!

Let's try and work through the TS tantrums as best we can 😸

public/sw.js

Co-authored-by: Alex Magana <alex.magana@andela.com>

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

src/app/lib/utilities/ichefURL/index.js

src/app/components/Image/index.tsx

karinathomasbbc

Soz a couple more comments - I am happy to assist with any of the service worker stuff, since I was the last one to touch it!

karinathomasbbc · 2024-05-03T08:02:44Z

src/app/legacy/psammead/psammead-image/src/index.jsx

@@ -65,7 +64,8 @@ export const Img = props => {
          <StyledImg src={src} {...otherProps} />
        </StyledPicture>
      )}
-      {pageType !== FRONT_PAGE && (
+
+      {[FRONT_PAGE, HOME_PAGE].includes(pageType) && (


Should this be the inverse (based on the diff)

Suggested change

{[FRONT_PAGE, HOME_PAGE].includes(pageType) && (

{![FRONT_PAGE, HOME_PAGE].includes(pageType) && (

karinathomasbbc · 2024-05-03T08:03:47Z

src/sw.test.js

      `(`for $image is $expectedUrl`, async ({ image, expectedUrl }) => {
        ({ fetchEventHandler } = await import('./service-worker-test'));

        const event = {
-          request: new Request(image, { headers: { accept: 'webp' } }),
+          request: new Request(image, { headers: { accept: 'jpg' } }),


Let's just remove the header (as this is likely to be the case in browsers that don't support webp)

Suggested change

request: new Request(image, { headers: { accept: 'jpg' } }),

request: new Request(image),

karinathomasbbc · 2024-05-03T08:06:07Z

src/sw.test.js

-        ${`${BASE_IMAGE_URL}/news/amz/puppies.jpeg`}          | ${{ accept: 'webp' }} | ${'image url must not include amz'}
-        ${`${BASE_IMAGE_URL}/news/worldservice/puppies.jpeg`} | ${{ accept: 'webp' }} | ${'image url must not include worldservice'}
-        ${`${BASE_IMAGE_URL}/news/puppies.jpg`}               | ${{}}                 | ${`webp not supported in request headers`}
+        image                                                      | headers               | reason


I think we should remove the headers column from the table now - given that webp is the default, then we can probably hardcode it below. This is to try and get coverage for all other scenarios (and some of them may even be invalid now)

src/sw.test.js

karinathomasbbc · 2024-05-03T08:10:37Z

src/sw.test.js

+      // The service worker doesn't do anything when webp is supported
+      it.each`
+        image
+        ${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`}


Could we also add a couple more scenarios here so that we have coverage of all scenarios (look at the test above) e.g. urls with /ace/standard etc - we may also want to include a column for the reason i.e. why the image is not requested

karinathomasbbc · 2024-05-03T08:12:01Z

src/app/components/Image/index.tsx

-    if ((!hasFallback && srcSet) || pageType !== FRONT_PAGE) return sizes;
+    if (
+      (!hasFallback && srcSet) ||
+      (pageType !== FRONT_PAGE && pageType !== HOME_PAGE)


Suggested change

(pageType !== FRONT_PAGE && pageType !== HOME_PAGE)

(![FRONT_PAGE, HOME_PAGE].includes(pageType))

karinathomasbbc · 2024-05-03T08:14:02Z

src/sw.test.js

Happy to help out with the tests for this (since I wrote them)! I'm keen to ensure that we get as close as possible to 100% code coverage.

Let's look at this after the catch up session next week.

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

…test with a url type

…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh

…-url-downgrade-changes WSTEAMA-749: Update service worker tests and URL generation logic

…-images-in-simorgh

… for that

…-images-in-simorgh

andrewscfc · 2024-05-23T12:12:40Z

cypress/e2e/pages/frontPage/testsForCanonicalOnly.js

@@ -3,6 +3,43 @@ import runCanonicalAdsTests from '../../../support/helpers/adsTests/testsForCano

 // For testing features that may differ across services but share a common logic e.g. translated strings.
 export const testsThatFollowSmokeTestConfigForCanonicalOnly = ({ service }) => {
+  describe(`Images`, () => {
+    it('should have a picture tag around images', () => {
+      cy.get('img').each($img => {


Don't know if it matters much but where does the $ variable convention come from? Not sure if linting might have picked it up?

andrewscfc · 2024-05-23T12:17:26Z

cypress/e2e/pages/frontPage/testsForCanonicalOnly.js

+          });
+      });
+    });
+  });


good coverage here but these could definitely be covered by integration tests I think. If they don't have a major impact on cypress runtime we could consider keeping them but if not we'd be better favouring integration tests.

andrewscfc · 2024-05-23T12:18:11Z

cypress/e2e/pages/testsForAllAMPPages.js

+    describe(
+      'Image Tests',
+      {
+        retries: 3,


are the retries necessary

andrewscfc · 2024-05-23T12:18:45Z

cypress/e2e/pages/testsForAllAMPPages.js

+
+              // Images are lazy loaded so we need to scroll to them, check they have loaded before getting currentSrc
+              cy.wrap($img)
+                .scrollIntoView()


is this why it needs to be a cypress test?

andrewscfc · 2024-05-23T12:22:07Z

public/sw.js

+    const supportsWebp =
+      req.headers.has('accept') && req.headers.get('accept').includes('webp');
+
+    // if supports webp is false in request header then don't use it?


Suggested change

// if supports webp is false in request header then don't use it?

// if supports webp is false in request header then don't use it

Guess this isn't a question?

andrewscfc · 2024-05-23T14:10:12Z

src/app/lib/utilities/ichefURL/index.js

@@ -11,40 +31,33 @@ const buildPlaceholderSrc = (src, resolution) => {
  const urlParts = imageSrc.replace(/https?:\/\//g, '').split('/');
  const [domain, mediaType, imgService, ...remainingUrlParts] = urlParts;
  const remainingUrlPartsWithoutResolution = remainingUrlParts.slice(1);
+  const webpImgUrl = `${remainingUrlPartsWithoutResolution}.webp`;


Suggested change

const webpImgUrl = `${remainingUrlPartsWithoutResolution}.webp`;

here is an array that may contain multiple elements, I'm surprised this passes tests actually?

I've suggested below we append the .webp after join below?

andrewscfc · 2024-05-23T14:10:40Z

src/app/lib/utilities/ichefURL/index.js

-    newResolution,
-    ...remainingUrlPartsWithoutResolution,
-  ];
+  const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl];


Suggested change

const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl];

const newUrl = [domain, mediaType, imgService, newResolution, ...remainingUrlPartsWithoutResolution];

andrewscfc · 2024-05-23T14:11:03Z

src/app/lib/utilities/ichefURL/index.js

-    newResolution,
-    ...remainingUrlPartsWithoutResolution,
-  ];
+  const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl];
  return `https://${newUrl.join('/')}`;


Suggested change

return `https://${newUrl.join('/')}`;

return `https://${newUrl.join('/')}.webp`;

andrewscfc · 2024-05-23T15:15:32Z

src/app/lib/utilities/srcSet/index.test.js

      },
      summary:
-        'should return a srcset with test in originCode and testland in location',
+        'should return a srcset with test in originCode and testland in location', // why was fallbackMimeType set to null in this test?


has this question been answered?

andrewscfc · 2024-05-23T15:17:31Z

src/app/lib/utilities/srcSet/index.test.js

+      },
+      summary:
+        'should return no file extension when none on locator and no MIME types', // do we need this test to cover null mime types?
+    },


Is this a real world scenario?

webp as default and removed when not available

3efe0cc

LilyL0u marked this pull request as ready for review April 29, 2024 15:13

LilyL0u and others added 10 commits April 29, 2024 16:14

Merge branch 'latest' into WSTEAMA-749-implement-webp-support-for-all…

73eedbc

…-images-in-simorgh

remove unused argument to ichef function

710449a

Merge branch 'WSTEAMA-749-implement-webp-support-for-all-images-in-si…

d1e1788

…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh

remove console logs

545e59f

another console log removed

7fbd1de

fix StoryPromo test by adding .webp now it is expected

748240e

add .webp to IndexPageSection src

56a9777

update snapshots

3494406

forgot to save the adding webp to storyPromo test urls

bbc7ee4

Merge branch 'latest' into WSTEAMA-749-implement-webp-support-for-all…

0436b6e

…-images-in-simorgh

alex-magana reviewed Apr 30, 2024

View reviewed changes

src/app/lib/utilities/ichefURL/index.js Outdated Show resolved Hide resolved

karinathomasbbc reviewed May 1, 2024

View reviewed changes

andrewscfc reviewed May 1, 2024

View reviewed changes

public/sw.js Outdated Show resolved Hide resolved

LilyL0u and others added 3 commits May 1, 2024 15:38

Update src/app/lib/utilities/ichefURL/index.js

465ade9

Co-authored-by: Alex Magana <alex.magana@andela.com>

Update public/sw.js

5066b2d

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

Update public/sw.js

4773611

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

karinathomasbbc reviewed May 1, 2024

View reviewed changes

src/app/lib/utilities/ichefURL/index.js Outdated Show resolved Hide resolved

src/app/lib/utilities/ichefURL/index.js Outdated Show resolved Hide resolved

andrewscfc reviewed May 1, 2024

View reviewed changes

src/app/components/Image/index.tsx Show resolved Hide resolved

LilyL0u added 4 commits May 1, 2024 19:22

some responses to review comments

1c28f0c

changing tests for sw

fba9d32

ts error fixed and test changed to webp

222617a

byline uses .png.webp

30d1a90

karinathomasbbc reviewed May 3, 2024

View reviewed changes

LilyL0u and others added 4 commits May 3, 2024 12:44

Update src/sw.test.js

3538734

Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>

fix psammead image logic to show img on non home/front pages and fix …

17bfaf2

…test with a url type

Merge branch 'WSTEAMA-749-implement-webp-support-for-all-images-in-si…

4cf07d3

…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh

Ensure headers are passed to the Request object

178a0f1

LilyL0u and others added 29 commits May 4, 2024 10:47

logic changes in sw and ichefurl, sw tests changed

cb9fe80

Merge branch 'WSTEAMA-749-implement-webp-support-for-all-images-in-si…

a448df1

…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh

Update service worker URL filter and associated tests

cdee302

Add URL checks for adding .webp extension

fb91fcd

Fix amz regex

b220666

Use nono-capture group for performance since only matches are required

6c4dc62

Add tests for ichef URL generation

af64a4b

Remove redundant tests

219514c

Ensure amz/worldservice does not apply to the news subdomain

de0b225

Refine web support check to exclude amz/worldservice for news via regex

be69d17

Ensure .webp is not appended to the URL if already included

b4a75b7

Add guide comment

097b2af

Use the pattern in the service worker to match .webp images

dbc1398

working through test fixes

0a98d99

Change service worker to match the images ichef project

5a30985

Set sad path tests in an isolated block

d2157c5

Merge pull request #11595 from bbc/WSTEAMA-749-implement-webp-support…

01aceaa

…-url-downgrade-changes WSTEAMA-749: Update service worker tests and URL generation logic

Merge branch 'latest' into WSTEAMA-749-implement-webp-support-for-all…

831a9f4

…-images-in-simorgh

srcset tests and make fallback not become webp

a5d9cab

remove fallback differentiation and update snapshots

61f7ee3

remove .webp from fallback in createSrcSet

7012533

add webp to placeholder and update placeholder test

b192568

update integration snapshots

2ca67d7

cypress test for images on home page, message banner is webp and test…

1419e39

… for that

cypress tests for images canonical and amp

4b0dd64

more image tests

576fb62

Merge branch 'latest' into WSTEAMA-749-implement-webp-support-for-all…

dd477ab

…-images-in-simorgh

more updated snapshots

e6966e9

update placeholder tests to expect webp

716d701

andrewscfc reviewed May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575

WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575

LilyL0u commented Apr 29, 2024 •

edited

alex-magana Apr 30, 2024 •

edited

karinathomasbbc May 1, 2024

LilyL0u May 1, 2024

karinathomasbbc May 1, 2024

karinathomasbbc May 1, 2024

karinathomasbbc May 1, 2024

andrewscfc May 1, 2024 •

edited

LilyL0u May 1, 2024

andrewscfc May 2, 2024 •

edited

LilyL0u May 2, 2024

andrewscfc May 3, 2024

karinathomasbbc May 1, 2024

karinathomasbbc May 1, 2024

karinathomasbbc May 1, 2024

LilyL0u May 1, 2024

karinathomasbbc May 1, 2024

karinathomasbbc left a comment

karinathomasbbc May 3, 2024

karinathomasbbc May 3, 2024

karinathomasbbc May 3, 2024

karinathomasbbc May 3, 2024

karinathomasbbc May 3, 2024

karinathomasbbc May 3, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

andrewscfc May 23, 2024

	(pageType === FRONT_PAGE \|\| pageType === HOME_PAGE) && (
	([FRONT_PAGE, HOME_PAGE].includes(pageType) && (

	{(pageType === FRONT_PAGE \|\| pageType === HOME_PAGE)(
	{([FRONT_PAGE, HOME_PAGE].includes(pageType))(

	{pageType !== FRONT_PAGE && pageType !== HOME_PAGE && (
	{![FRONT_PAGE, HOME_PAGE].includes(pageType) && (

	{[FRONT_PAGE, HOME_PAGE].includes(pageType) && (
	{![FRONT_PAGE, HOME_PAGE].includes(pageType) && (

	request: new Request(image, { headers: { accept: 'jpg' } }),
	request: new Request(image),

	(pageType !== FRONT_PAGE && pageType !== HOME_PAGE)
	(![FRONT_PAGE, HOME_PAGE].includes(pageType))

	// if supports webp is false in request header then don't use it?
	// if supports webp is false in request header then don't use it

	const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl];
	const newUrl = [domain, mediaType, imgService, newResolution, ...remainingUrlPartsWithoutResolution];

	return `https://${newUrl.join('/')}`;
	return `https://${newUrl.join('/')}.webp`;

WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575

Are you sure you want to change the base?

WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575

Conversation

LilyL0u commented Apr 29, 2024 • edited

Testing

Helpful Links

alex-magana Apr 30, 2024 • edited

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

andrewscfc May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewscfc May 2, 2024 • edited

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

karinathomasbbc left a 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

LilyL0u commented Apr 29, 2024 •

edited

alex-magana Apr 30, 2024 •

edited

andrewscfc May 1, 2024 •

edited

andrewscfc May 2, 2024 •

edited