Comments (9)
About keeping intersecting images:
The issue comes from the beacon script in the LCPCandidates
: https://github.com/wp-media/wp-rocket/blob/b459f191a348426b2b1a3cf656a4db577e76800f/assets/js/lcp-beacon.js#L9C1-L16C12
This condition is checking that the image is fully in the viewport. Instead, we should check that it is intersecting only. We have such a check already for the ATF images here.
- We should make a dedicated function
isIntersecting(rect)
returning a boolean based on the check already existing, and use this in both here and here.
About accounting only for the visible area for LCP "score"
The area to account for is computed here. It is currently accounting for the complete image size.
To account for the visible part only, we should do:
const visibleWidth = Math.min(rect.width, (window.innerWidth || document.documentElement.clientWidth) - rect.left));
const visibleHeight = Math.min(rect.height, (window.innerHeight || document.documentElement.clientHeight) - rect.top))
const area = visibleWidth * visibleHeight;
For the testing part, TBC with @jeawhanlee but it might be more suited to test this with Rocket-E2E.
Effort: [XS] (test scenarios are outside the scope as they are already on Rocket-E2E, if we will use Rocket-E2E).
from wp-rocket.
Should this be moved to Ready for Review?
from wp-rocket.
@Miraeld @wp-media/qa-team Do we have test scenarios, maybe on Rocket-E2E to validate this? Maybe similar to the acceptance criteria?
from wp-rocket.
@MathieuLamiot not from what I am aware of.
from wp-rocket.
@MathieuLamiot , @Miraeld just pushed these 2 templates on /wp-media/rocket-test-data:
lcp_images_intersecting_viewport_A.php
lcp_images_intersecting_viewport_B.php
that should cover the AC.
Will provide you with pages created on e2e as well, in a moment.
from wp-rocket.
@MathieuLamiot , @Miraeld
on e2e, scenario 1
ImageA (size: 200x200, fully in the viewport) and ImageB (size:300x300, only 300x1 visible in the viewport)
we have page link)
and for scenario 2
ImageA (size: 200x200, fully in the viewport) and ImageB (size:300x300, only 300x250 visible in the viewport)
we have page link
TC link.
from wp-rocket.
@Miraeld Have you performed basic checks to ensure the code does not break on happy path and that the reported failing cases are fixed? In which case, we can merge so that we only have to work on the feature branch. OK? Thanks 🙏
from wp-rocket.
Kind reminder @Miraeld: can this be merged?
from wp-rocket.
I did basic check yea, and it worked.
I think it can be merged yes.
from wp-rocket.
Related Issues (20)
- Change the way the description for the Clear button is displayed
- Add query string to force LRC hash injection HOT 3
- [DEV] Enable requireFileExists rule in phpstan
- rocket_lrc_processed_tags should be case insensitive HOT 1
- 3.17 Regex processor - Optimize the regex
- 3.17 - Maybe use libxml_get_errors or something else to identify DOMDocument modifications and bail out HOT 1
- 3.17 - Identify unwanted body modification from DOMDocument to bail out early HOT 1
- 3.17 - Replace HTML Write operation from DOMDocument with Regex for LRC hash injection
- Avoid use of multiple Template redirect action
- Auto-exclude Trust Index Google Widget CSS from LazyLoad for CSS Background Images HOT 3
- OCI: when using a CDN that's a subdomain cdn.site.com, imagesrcset urls inside preload tag are not replaced to use the CDN cname HOT 1
- UI issue after clear cache and apply LRC with certain template HOT 6
- Beacon script can be injected multiple times when there are multiple </body> in the page.
- LRC Hash are not injected on tags with special characters
- LRC hashes are injected to the first found matching tag, which might not be the targeted one
- OCI & LRC optimizations should not be applied, even if available, on SaaS visits
- Safeguard DOMDocument empty HTML
- Updating menu and widgets, and WP Rocket->Clear cache results in 502 timeout
- Typo in French translation HOT 3
- wpr-beacon.min.js is added even if DONOTROCKETOPTIMIZE is set to true
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from wp-rocket.