Closes #6638: Handle Properly picture tag #6659

Miraeld · 2024-05-24T11:51:30Z

Description

Documentation

User documentation

First of all, this PR brings a fix for the LCP candidates when it's a <picture> tag detected. Before, we would also detect the sub-tag <img> and both would compete each other leading to errors.
Then, it also brings another way to process <picture> tag when there is a type attribute in order to preload the first <source> image.

Technical documentation

Type of change

Enhancement (non-breaking change which improves an existing functionality).

New dependencies

Risks

Checklists

Feature validation

I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
I triggered all changed lines of code at least once without new errors/warnings/notices.
I implemented built-in tests to cover the new/changed code.

Documentation

I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

I wrote self-explanatory code about what it does.
I wrote comments to explain why it does it.
I named variables and functions explicitely.
I protected entry points against unexpected inputs.
I did not introduce unecessary complexity.
I listed the introduced external dependencies explicitely on the PR.
I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

I handled errors when needed.
I wrote user-facing messages that are understandable and provide actionable feedbacks.
I prepared ways to observe the implemented system (logs, data, etc.).

Risks

I explicitely mentioned performance risks in the PR.
I explicitely mentioned security risks in the PR.

codacy-production · 2024-05-24T12:07:04Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `39b8f18`¹	✅ 100.00% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`39b8f18`)	Report Missing	Report Missing	Report Missing
Head commit (`bab312a`)	37228	14421	38.74%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6659)	8	8	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

inc/Engine/Media/AboveTheFold/Frontend/Controller.php

Fix the empty href & duplicated `min-width` with picture tag

Tabrisrp · 2024-05-27T23:10:29Z

Need to lint the code a bit

Miraeld · 2024-05-28T13:06:11Z

My bad, didn't see that failed CI.
Done :)

Mai-Saad · 2024-05-29T06:29:22Z

Test plan:

run e2e tests => no regression
check the fetch priority is correct + images excluded from LL for mobile and desktop:
Different media (AC Preload the picture markup #6530 (comment)) ==> https://e2e.rocketlabsqa.ovh/lcp_picture_template2/ ✔️ ,
https://e2e.rocketlabsqa.ovh/lcp_picture_template/ => having min-width only is agreed as edge case now https://wp-media.slack.com/archives/CUT7FLHF1/p1716990574011419?thread_ts=1716965466.492079&cid=CUT7FLHF1,
https://e2e.rocketlabsqa.ovh/lcp_picture_relative/
Different type (only 1st type preloaded in 1st 2 templates, AC Correctly handle image types when preloading LCP images #6638 (comment)) ==> https://e2e.rocketlabsqa.ovh/lcp_picture_type/, https://e2e.rocketlabsqa.ovh/lcp_picture_responsive_type/, https://e2e.rocketlabsqa.ovh/lcp_picture_media_type_mixed/

…p-preload

Miraeld · 2024-05-31T01:03:01Z

Good morning @Mai-Saad & @piotrbak ,

Latest commits are adding the following:

The newest AC (Correctly handle image types when preloading LCP images #6638 (comment)),
Fix tests, each AC are included in integration tests, however something was wrong with them, and they were not running. Now it's running and everything is passing.

Also, e2e is passing on my side, everything ok.

MathieuLamiot · 2024-06-05T21:22:07Z

@Miraeld I see you moved this to Ready for Review.
But it is not up-to-date with develop and there are no E2E results shared for this PR updated on develop. Can you make sure to provide all this?

…p-preload

Miraeld · 2024-06-05T21:29:27Z

The codebase is ready for review,
I'm trying to run e2e, but can't get it working (the whole e2e app), non related to this PR.

And it's just been updated with develop latest changes.

Mai-Saad · 2024-06-06T07:45:55Z

@wordpressfan Thanks for sharing markups above.

For the 1st markup


	<picture class="core-image">
		<source type="image/webp" media='(max-width: 500px)' srcset="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-400x600.webp">
		<source type="image/webp" media='(min-width: 501px) and (max-width: 768px)' srcset="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-768x513.webp">
		<img class="skip-lazy" src="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" alt="Kwiatowy księżyc" title="Kwiatowy księżyc">
	</picture>

Update: it's 2nd source missing and thus min-width is not correct thanks to @Miraeld

it's missing to preload <img> + and max-width of 2nd image => clear on this page https://new.rocketlabsqa.ovh/lcp_picture_media_type_mixed/. @wp-media/productrocket it contains both min-width and max-width, please confirm expected here. (now no empty href nor duplicated min-width but we didn't preload img tag and we removed and max-width from 2nd image)
Expected markup as per original issue #6669:

<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-400x600.webp" media="(max-width: 500px)" fetchpriority="high">
<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-768x513.webp" media="(min-width: 501px) and (max-width: 768px)" fetchpriority="high">
<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" alt="Kwiatowy księżyc" media="(min-width: 768.1px)" fetchpriority="high">

2nd markup based on Preload the picture markup #6530 is 👍 (test page: https://new.rocketlabsqa.ovh/lcp_picture_template2/)
3rd markup, based on Correctly handle image types when preloading LCP images #6638 (comment) is 👍 (test page: https://new.rocketlabsqa.ovh/lcp_picture_responsive_type_template/)
4th markup, based on 2nd point here Correctly handle image types when preloading LCP images #6638 (comment) is 👍 (test page https://new.rocketlabsqa.ovh/lcp_picture_type/)

MathieuLamiot · 2024-06-06T08:34:05Z

E2E results: https://wp-media.slack.com/archives/CUT7FLHF1/p1717653110913929?thread_ts=1717624428.972699&cid=CUT7FLHF1
Moving this to Ready for Review. And pending Product feedback.

Miraeld · 2024-06-06T13:56:27Z

I was checking on develop,
while using the HTML markup:

	<picture class="core-image">
		<source type="image/webp" media='(max-width: 500px)' srcset="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-400x600.webp">
		<source type="image/webp" media='(min-width: 501px) and (max-width: 768px)' srcset="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-768x513.webp">
		<img class="skip-lazy" src="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" alt="Kwiatowy księżyc" title="Kwiatowy księżyc">
	</picture>

@piotrbak for everything related to the only markup @Mai-Saad mentioned was wrong, on develop we are getting the following preload tag:

<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" fetchpriority="high">

What we are getting on this PR:

<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-400x600.webp" media="(max-width: 500px)" fetchpriority="high">
<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" media="(min-width: 500.1px)" fetchpriority="high">

Not sure if this is a regression or not what we are getting with this PR.
What is the actual expected behavior for this ? Is @Mai-Saad correct in her latest comment :

<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-400x600.webp" media="(max-width: 500px)" fetchpriority="high">
<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-768x513.webp" media="(min-width: 501px) and (max-width: 768px)" fetchpriority="high">
<link rel="preload" as="image" href="https://variance.pl/wp-content/uploads/2024/05/Kwiatowy-Ksiezyc-1348x900.webp" alt="Kwiatowy księżyc" media="(min-width: 768.1px)" fetchpriority="high">

I was thinking to move this case out of this PR, so everything else can move forward while this one is being worked on.
What do you think ?

EDIT:
After looking into it a bit, it isn't the <img tag that isn't preloaded, but the second <source> tag.
This is due to this bail-out here, which was introduce to tackle another case of the picture tag.

…p-preload

codacy-production · 2024-06-06T23:48:11Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `4dd69dd`¹	✅ 100.00% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`4dd69dd`)	Report Missing	Report Missing	Report Missing
Head commit (`89f3948`)	37263	14459	38.80%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6659)	14	14	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

Miraeld · 2024-06-06T23:50:54Z

@Mai-Saad ,
As per discussion here, we can move forward with this PR, while fixing the remaining markup in another issue here

…o fix/picture-markup-type-bailout

…ure-src-regression

Miraeld · 2024-06-07T13:01:06Z

@MathieuLamiot , @Mai-Saad ,
This PR Now includes changes from #6708, & #6709.
Which means the picture tag should work properly now, on every aspect + the src regression is fixed.

Mai-Saad · 2024-06-08T08:53:29Z

@Miraeld Thanks for the update.
Tested templates:

https://new.rocketlabsqa.ovh/lcp_picture_media_type_mixed/ 👍
https://new.rocketlabsqa.ovh/lcp_picture_relative/ 👍
https://new.rocketlabsqa.ovh/lcp_picture_responsive_type/ 👍
https://new.rocketlabsqa.ovh/lcp_picture_template2/ 👍
https://new.rocketlabsqa.ovh/lcp_picture_type/ 👍
https://new.rocketlabsqa.ovh/lcp_bg_multimage_template/ 👍
https://new.rocketlabsqa.ovh/lcp_responsive_image_template/ 👍 (noted here and already on trunk, if img not having sizes we will have empty imagesizes in preload which will cause markup error, however, already on nowprocket, not having size to responsive image was causing error) ==> we can update the test template later to ensure original markup is valid
General note: markup with only media='min-width' will only preload srcset now (accepted as edge case for now) https://new.rocketlabsqa.ovh/lcp_picture_template/

Possible breaking markups (not sure if it's common, needs further investigation and expected o/p from @wp-media/productrocket if we will handle any in new GH maybe):

test page https://new.rocketlabsqa.ovh/lcp_picture_responsive_type_mixed/

     <picture   class="wp-image-8418">
   <source Type="image/avif" Media = '(max-width: 500px)' Srcset="https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1024x576.jpg.avif 1024w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-300x169.jpg.avif 300w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-768x432.jpg.avif 768w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1536x864.jpg.avif 1536w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1200x675.jpg.avif 1200w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-600x338.jpg.avif 600w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia.jpg.avif 1920w" sizes="(max-width: 1024px) 100vw, 1024px">
   <source type = "image/webp" Media='(max-width: 800px)' SRCSET="https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1024x576.jpg.webp 1024w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-300x169.jpg.webp 300w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-768x432.jpg.webp 768w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1536x864.jpg.webp 1536w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1200x675.jpg.webp 1200w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-600x338.jpg.webp 600w" sizes="(max-width: 1024px) 100vw, 1024px">
   <img  width="1024" height="576" Src="https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1024x576.jpg" alt="" srcset="https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1024x576.jpg 1024w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-300x169.jpg 300w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-768x432.jpg 768w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1536x864.jpg 1536w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-1200x675.jpg 1200w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia-600x338.jpg 600w, https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-—-kopia.jpg 1920w" sizes="(max-width: 1024px) 100vw, 1024px">
</picture>

will be preloaded as

<link rel="preload" as="image" href="https://imagify.rocketlabsqa.ovh/wp-content/uploads/2024/05/home-new-bg-free-img-%E2%80%94-kopia-1024x576.jpg" media="(min-width: 500.1px)" fetchpriority="high">

2- using max-height instead of max-width https://new.rocketlabsqa.ovh/lcp_picture_template2_maxheight/

<picture class="picture_test2"> 
			<source srcset="<?php echo content_url().'/rocket-test-data/images/paper.jpeg' ?>" media="(max-height: 400px)"> 
			<source  srcset="<?php echo content_url().'/rocket-test-data/images/lcp/testjpeg.jpeg' ?>" media="(max-height: 800px)"> 
			<img src="<?php echo content_url().'/rocket-test-data/images/lcp/testwebp.webp'?>"> 	
		</picture>

will be

<link rel="preload" as="image" href="https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/paper.jpeg" media="(max-height: 400px)" fetchpriority="high">
<link rel="preload" as="image" href="https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testjpeg.jpeg" media="(max-height: 800px)" fetchpriority="high">

e2e results https://wp-media.slack.com/archives/CUT7FLHF1/p1717836779879489?thread_ts=1717754667.187049&cid=CUT7FLHF1
Recommend rerun e2e and if all good + we won't handle markups above here then we can merge

MathieuLamiot · 2024-06-08T17:11:37Z

Re-ran E2E to confirm: all good (details here).

Only waiting for @piotrbak @DahmaniAdame confirmation on the 2 points from @Mai-Saad above.

piotrbak · 2024-06-10T10:00:14Z

@MathieuLamiot @Mai-Saad We're good with those

Miraeld added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label May 24, 2024

Miraeld added this to the 3.16.1 milestone May 24, 2024

Miraeld requested a review from a team May 24, 2024 11:51

Miraeld self-assigned this May 24, 2024

Miraeld added 4 commits May 24, 2024 19:00

Fix subtag <img> of <picture> being picked-up as lcp candidates

d8794a5

modify process of <picture> tag to comply AC

f427ac1

Add tests

c872a7b

Fix conflicts

3a16cb0

Miraeld force-pushed the enhancement/6638-handle-picture-tag-in-lcp-preload branch from 2d0f5d6 to 3a16cb0 Compare May 24, 2024 12:01

Fix linter

c98ecbd

Tabrisrp requested changes May 27, 2024

View reviewed changes

inc/Engine/Media/AboveTheFold/Frontend/Controller.php Outdated Show resolved Hide resolved

comply feedback

8cd92f6

Miraeld requested a review from Tabrisrp May 27, 2024 20:30

Miraeld mentioned this pull request May 27, 2024

Picture markup with media isnot handled correctly #6660

Closed

Closes #6660: Handle media properly in <picture>

d80f88e

Miraeld force-pushed the enhancement/6638-handle-picture-tag-in-lcp-preload branch from 5efbb83 to d80f88e Compare May 27, 2024 21:11

Closes #6669:

c49ac6d

Fix the empty href & duplicated `min-width` with picture tag

Miraeld changed the title ~~Enhancement/6638 handle picture tag in lcp preload~~ Closes #6638: Handle Properly picture tag May 27, 2024

Miraeld mentioned this pull request May 27, 2024

href attribute and min-width and max-width properties are not handled properly when using the media attribute #6669

Closed

This was linked to issues May 27, 2024

Picture markup with media isnot handled correctly #6660

Closed

href attribute and min-width and max-width properties are not handled properly when using the media attribute #6669

Closed

fix linter

1709ee6

Tabrisrp approved these changes May 28, 2024

View reviewed changes

Miraeld added 2 commits May 31, 2024 06:51

Fix last AC

65bb120

Merge branch 'develop' into enhancement/6638-handle-picture-tag-in-lc…

0026b97

…p-preload

MathieuLamiot requested a review from a team June 5, 2024 14:47

Merge branch 'develop' into enhancement/6638-handle-picture-tag-in-lc…

0aa0cf1

…p-preload

Mai-Saad mentioned this pull request Jun 6, 2024

Fallback value for src attribute if not set #6700

Merged

22 tasks

Miraeld added 3 commits June 7, 2024 06:41

Fix last picture error when we are preloading lcp

5388682

Merge branch 'develop' into enhancement/6638-handle-picture-tag-in-lc…

a53879c

…p-preload

Update lcp-beacon

89f3948

Miraeld mentioned this pull request Jun 6, 2024

Picture markup not preloading properly with LCP #6707

Closed

Miraeld and others added 5 commits June 7, 2024 06:52

Merge branch 'enhancement/6638-handle-picture-tag-in-lcp-preload' int…

4cd5316

…o fix/picture-markup-type-bailout

Add back src for picture and img-srcset

91fdd3b

Merge commit '4cd5316a7b07ad8ee07e207760365dbc07553ed8' into fix/pict…

e9c1717

…ure-src-regression

Merge with picture fix PR and gulp

701b7a8

fixl inter

6026c28

Miraeld mentioned this pull request Jun 7, 2024

Closes #6707: Picture markup not preloading properly with LCP #6708

Closed

14 tasks

Miraeld linked an issue Jun 7, 2024 that may be closed by this pull request

Picture markup not preloading properly with LCP #6707

Closed

jeawhanlee approved these changes Jun 7, 2024

View reviewed changes

Khadreal added this pull request to the merge queue Jun 10, 2024

Merged via the queue into develop with commit c450ed1 Jun 10, 2024
23 checks passed

Khadreal deleted the enhancement/6638-handle-picture-tag-in-lcp-preload branch June 10, 2024 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #6638: Handle Properly picture tag #6659

Closes #6638: Handle Properly picture tag #6659

Miraeld commented May 24, 2024 •

edited

codacy-production bot commented May 24, 2024 •

edited

Tabrisrp commented May 27, 2024

Miraeld commented May 28, 2024

Mai-Saad commented May 29, 2024 •

edited

Miraeld commented May 31, 2024

MathieuLamiot commented Jun 5, 2024

Miraeld commented Jun 5, 2024

Mai-Saad commented Jun 6, 2024 •

edited

MathieuLamiot commented Jun 6, 2024

Miraeld commented Jun 6, 2024 •

edited

codacy-production bot commented Jun 6, 2024

Miraeld commented Jun 6, 2024

Miraeld commented Jun 7, 2024 •

edited

Mai-Saad commented Jun 8, 2024 •

edited

MathieuLamiot commented Jun 8, 2024

piotrbak commented Jun 10, 2024

Closes #6638: Handle Properly picture tag #6659

Closes #6638: Handle Properly picture tag #6659

Conversation

Miraeld commented May 24, 2024 • edited

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Documentation

Code style

Observability

Risks

codacy-production bot commented May 24, 2024 • edited

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

Tabrisrp commented May 27, 2024

Miraeld commented May 28, 2024

Mai-Saad commented May 29, 2024 • edited

Miraeld commented May 31, 2024

MathieuLamiot commented Jun 5, 2024

Miraeld commented Jun 5, 2024

Mai-Saad commented Jun 6, 2024 • edited

MathieuLamiot commented Jun 6, 2024

Miraeld commented Jun 6, 2024 • edited

codacy-production bot commented Jun 6, 2024

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

Miraeld commented Jun 6, 2024

Miraeld commented Jun 7, 2024 • edited

Mai-Saad commented Jun 8, 2024 • edited

MathieuLamiot commented Jun 8, 2024

piotrbak commented Jun 10, 2024

Miraeld commented May 24, 2024 •

edited

codacy-production bot commented May 24, 2024 •

edited

Mai-Saad commented May 29, 2024 •

edited

Mai-Saad commented Jun 6, 2024 •

edited

Miraeld commented Jun 6, 2024 •

edited

Miraeld commented Jun 7, 2024 •

edited

Mai-Saad commented Jun 8, 2024 •

edited