-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575
base: latest
Are you sure you want to change the base?
WSTEAMA-654 Reinstate webp image support on all WS pages (pt.1) #11575
Conversation
…-images-in-simorgh
…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh
…-images-in-simorgh
public/sw.js
Outdated
if (req.headers.has('accept')) { | ||
supportsWebp = req.headers.get('accept').includes('webp'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
if (req.headers.has('accept')) { | |
supportsWebp = req.headers.get('accept').includes('webp'); | |
} | |
const supportsWebp = req.headers.has('accept') && req.headers.get('accept').includes('webp'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
public/sw.js
Outdated
if (req.headers.has('accept')) { | ||
supportsWebp = req.headers.get('accept').includes('webp'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/app/components/Image/index.tsx
Outdated
// I don't know if we need this???? | ||
// if (!hasFallback) return srcSet; | ||
// if (pageType !== FRONT_PAGE && pageType !== HOME_PAGE) | ||
// return fallbackSrcSet; | ||
// return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we just leave this in to be on the safe side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to remove this as we want to return a webp srcset by default now?
| fallbackSrcSet? | string | the fallback srcset (probably the jpeg images) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
</> | ||
)} | ||
{hasFallback && | ||
(pageType === FRONT_PAGE || pageType === HOME_PAGE) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this a wee bit easier to read
(pageType === FRONT_PAGE || pageType === HOME_PAGE) && ( | |
([FRONT_PAGE, HOME_PAGE].includes(pageType) && ( |
@@ -50,7 +50,7 @@ export const Img = props => { | |||
|
|||
return ( | |||
<> | |||
{pageType === FRONT_PAGE && ( | |||
{(pageType === FRONT_PAGE || pageType === HOME_PAGE)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{(pageType === FRONT_PAGE || pageType === HOME_PAGE)( | |
{([FRONT_PAGE, HOME_PAGE].includes(pageType))( |
)} | ||
{pageType !== FRONT_PAGE && ( | ||
{pageType !== FRONT_PAGE && pageType !== HOME_PAGE && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{pageType !== FRONT_PAGE && pageType !== HOME_PAGE && ( | |
{![FRONT_PAGE, HOME_PAGE].includes(pageType) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally tried this and typescript had a tantrum. Will try again as your syntax may be slightly different, let's see!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try and work through the TS tantrums as best we can 😸
Co-authored-by: Alex Magana <alex.magana@andela.com>
Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>
Co-authored-by: Karina Thomas <58214768+karinathomasbbc@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -65,7 +64,8 @@ export const Img = props => { | |||
<StyledImg src={src} {...otherProps} /> | |||
</StyledPicture> | |||
)} | |||
{pageType !== FRONT_PAGE && ( | |||
|
|||
{[FRONT_PAGE, HOME_PAGE].includes(pageType) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be the inverse (based on the diff)
{[FRONT_PAGE, HOME_PAGE].includes(pageType) && ( | |
{![FRONT_PAGE, HOME_PAGE].includes(pageType) && ( |
src/sw.test.js
Outdated
`(`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' } }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just remove the header (as this is likely to be the case in browsers that don't support webp
)
request: new Request(image, { headers: { accept: 'jpg' } }), | |
request: new Request(image), |
src/sw.test.js
Outdated
${`${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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
// The service worker doesn't do anything when webp is supported | ||
it.each` | ||
image | ||
${`${TEST_IMAGE_URL}/news/puppies.jpg.webp`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if ((!hasFallback && srcSet) || pageType !== FRONT_PAGE) return sizes; | ||
if ( | ||
(!hasFallback && srcSet) || | ||
(pageType !== FRONT_PAGE && pageType !== HOME_PAGE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(pageType !== FRONT_PAGE && pageType !== HOME_PAGE) | |
(![FRONT_PAGE, HOME_PAGE].includes(pageType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…-images-in-simorgh
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const webpImgUrl = `${remainingUrlPartsWithoutResolution}.webp`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right this code looks wrong now I look at it again 😂 Maybe this actually shows this code isn't tested! The test that is supposed to be testing the placeholder srcset is this
it.each(['mpv', 'pips'])( 'returns a placeholder image if no image src provided and origin code is %s', originCode => { expect(getIChefURL({ resolution: 512, originCode })).toEqual( 'https://ichef.bbci.co.uk/images/ic/512xn/p0b36kgx.png.webp', ); }, );
which uses this next:
if (originCode === 'mpv' || originCode === 'pips') { return buildPlaceholderSrc(locator, resolution); }
and then
const buildPlaceholderSrc = (src, resolution) => { const imageSrc = src || 'https://ichef.bbci.co.uk/images/ic/640xn/p0b36kgx.png';
but because the test doesn't provide a src, the hard coded url is always used in the test and this just happens to work as there is only one part in the 'remainingUrlParts'.
I shall change this.
newResolution, | ||
...remainingUrlPartsWithoutResolution, | ||
]; | ||
const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newUrl = [domain, mediaType, imgService, newResolution, webpImgUrl]; | |
const newUrl = [domain, mediaType, imgService, newResolution, ...remainingUrlPartsWithoutResolution]; |
}, | ||
summary: | ||
'should return no file extension when none on locator and no MIME types', // do we need this test to cover null mime types? | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a real world scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't knoe but as I un-nulled the mime types in the other tests I added a new test with them as null just in case that was important for some reason. I don't know if it represents something real or if the mime types were just null because they weren't originally important in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should remove a test if we aren't clear what the real world scenario is?
Co-authored-by: Andrew Bennett <andrew.bennett07@bbc.co.uk>
Co-authored-by: Andrew Bennett <andrew.bennett07@bbc.co.uk>
Co-authored-by: Andrew Bennett <andrew.bennett07@bbc.co.uk>
Co-authored-by: Andrew Bennett <andrew.bennett07@bbc.co.uk>
…morgh' of github.com:bbc/simorgh into WSTEAMA-749-implement-webp-support-for-all-images-in-simorgh
I have removed the code adding .webp to the message banner image because it doesn't have a fallback so this could be a problem for viewing home pages on browsers that don't support webp. |
…-images-in-simorgh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this PR, good work sticking with this as it really grew legs. There are few comments from me and others that may warrant consideration but don't want to block this.
}, | ||
summary: | ||
'should return no file extension when none on locator and no MIME types', // do we need this test to cover null mime types? | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should remove a test if we aren't clear what the real world scenario is?
}, | ||
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'should return a srcset with test in originCode and testland in location', // why was fallbackMimeType set to null in this test? | |
'should return a srcset with test in originCode and testland in location', |
We haven't resolved this mystery, lets drop the comment?
…-images-in-simorgh
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
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines