-
Notifications
You must be signed in to change notification settings - Fork 210
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
Fix issues with retina devices #6630
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more Footnotes
|
@wordpressfan Thanks for the PR, Can see the manual visit of mobile /desktop is added to atf now. however, during exploratory test, warmup after clear critical images is adding only the mobile version to ATF, is this something we will handle here? @piotrbak |
@Mai-Saad Could you check if visits from the saas are being made to the desktop? If so, it means that the PR brings regression |
@piotrbak While RUCSS is on, on trunk we are adding both desktop/mobile to ATF while on PR only mobile |
@wordpressfan I see in your latest version that you are using
Should we try with the earlier version? |
EDIT: very quick test with commit c35559c seems working.
@wordpressfan can you elaborate on cases that did not work with devicePixelRatio? |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more Footnotes
|
Yes it was working in some cases, but with me it wasn't properly working because of the threshold wasn't valid in this case, for example when u visit from Iphone SE with width 375px and height 667px, with the new code:
and this is out of our threshold, so I think we may need to think again about threshold values. You can use the dev tools mobile view to check that. I reverted the code back to this commit. |
Can confirm that saas visits are working now as on trunk. while manual mobile visits still not working for some like iPhone SE @wordpressfan mentioned above while for Samsung Galaxy S8+ it's working and adding to DB |
I think there is a misunderstansing here. So for Iphone SE, there are 375 CSS pixels and 750 physical pixels. The formula might be inverted? That being said I would advise not.to apply the threshold on the physical resolution butnon the CSS one. @piotrbak @DahmaniAdame Do we have the values for the failing cases originally reported by Mai? |
@MathieuLamiot u mean use multiply by not divided by, correct? I played with this also but Yes we may need to change the threshold also. |
Here are the details for a Samsung Galaxy s8+ as mentioned by @Mai-Saad |
@wordpressfan @Mai-Saad Based on this, we'll know what to do |
Are there device size that we shouldn't be bothered with or we need to support all device size ? |
The specification is always the same: for mobile devices, anything bigger than 393x830 must be discarded. Enregistrement de l’écran 2024-05-14 à 12.34.42 |
@MathieuLamiot on trunk both Samsung Galaxy s8+ and iPhone SE are not added to the database. on PR, Samsung Galaxy s8+ is working while iPhone SE is not. (theoretically any mobile dimensions <= 393x830 shall work as per our test cases) |
Ok, I managed to reproduce and this is much more tricky, but does not seem related to the script. Using dev tools to fake a Galaxy S8+, the innerHeight returned is not the same depending on the page! http://mathieu.e2e.rocketlabsqa.ovh/lcp_bg_samestyle_template/ I get 2015, so the physical pixel size. Which is rejected by the script. http://mathieu.e2e.rocketlabsqa.ovh/ I get 740, the expected value. Enregistrement.de.l.ecran.2024-05-14.a.12.44.13.movThis happens even without WP Rocket installed. |
This seem to happen on our templates. I can't reproduce on my personal website. I am wondering if the template loader plugin could be the cause of this as we interefere with the template_include hook |
I think I got it. When loading most WordPress sites, I can see this in the : But this is not in the templates we use for e2e website. I guess it is added by WordPress in its default templates, maybe as part of wp_head action? If I manually add it to our templates, everything works as expected. |
Closing the PR for now as we don't want to change anything compared to the current behavior. |
Description
Fixes #6628
Documentation
User documentation
Explain how this code impacts users.
Technical documentation
Explain how this code works. Diagram & drawings are welcomed.
Type of change
Delete options that are not relevant.
New dependencies
List any new dependencies that are required for this change.
Risks
List possible performance & security issues or risks, and explain how they have been mitigated.
Checklists
Feature validation
Documentation
Code style
Observability
Risks