Fix issues with retina devices

[wordpressfan]

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 feature (non-breaking change which adds functionality).
• [ ] Bug fix (non-breaking change which fixes an issue).
• [ ] Enhancement (non-breaking change which improves an existing functionality).
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as before).

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

• [ ] 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[bot]]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for c7a7149548afa175a43428adb1e33da790108d02[^1]	:white_check_mark: ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c7a7149548afa175a43428adb1e33da790108d02)	Report Missing	Report Missing	Report Missing
Head commit (b7f76d3f3e7c9a6e209c938d8724e593908177aa)	37183	14401	38.73%

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 (#6630)	0	0	∅ (not applicable)

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} [^1]: 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.

[Mai-Saad]

@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 with trunk with PR

[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

[Mai-Saad]

@piotrbak While RUCSS is on, on trunk we are adding both desktop/mobile to ATF while on PR only mobile

[MathieuLamiot]

@wordpressfan I see in your latest version that you are using screen.width rather than innerWidth and pixel ratio. Can you elaborate on why? I am afraid this will cause issues:

• with SaaS, we control the display size, not the screen size. Hence, probably why we have the regression spotted by @Mai-Saad.
• with human visits, screen.width will return the size of the actual physical screen I believe. If the viewport is smaller (window is made smaller by the user, or there are many toolbars at the top of the browser for instance), the website might display smaller but we would still account for the full screensize.

Should we try with the earlier version? return ( window.innerWidth || document.documentElement.clientWidth ) / ( window.devicePixelRatio || 1 );

[MathieuLamiot]

EDIT: very quick test with commit c35559c039626de66f196f799069fc0ded8169f3 seems working.

• SaaS visits trigger a row in DB with Mobile & Desktop
• Manual visit with a mobile trigger a row in the DB as well

@wordpressfan can you elaborate on cases that did not work with devicePixelRatio?

[codacy-production[bot]]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for c7a7149548afa175a43428adb1e33da790108d02[^1]	:white_check_mark: ∅ (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c7a7149548afa175a43428adb1e33da790108d02)	Report Missing	Report Missing	Report Missing
Head commit (f4887d01e74624670080c9f76b1c44b8c352e51a)	37183	14401	38.73%

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 (#6630)	0	0	∅ (not applicable)

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} [^1]: 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.

[wordpressfan]

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:

window.devicePixelRatio = 2
window.innerWidth = 375
New final width = 187.5

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.

[Mai-Saad]

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

[MathieuLamiot]

I think there is a misunderstansing here. innerWidth returns CSS pixels. The devicePixelRatio can be used to go from CSS pixels to Physical pixels.

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 Maybe we should just keep innerWidth and adjust thresholds?

Do we have the values for the failing cases originally reported by Mai?

[wordpressfan]

@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.

[MathieuLamiot]

Here are the details for a Samsung Galaxy s8+ as mentioned by @Mai-Saad The CSS resolution should be OK with our threshold. We should be getting 360 and a bit less than 740 as values to compare with the thresholds https://www.webmobilefirst.com/en/devices/samsung-galaxy-s8-plus/

[MathieuLamiot]

@wordpressfan @Mai-Saad To know exactly what to do, we need to pick a phone model, get its online specs (CSS pixel and screen size) from the website I shared above for instance. Then, check the returned value for this model in innerWidth.

Based on this, we'll know what to do

[wordpressfan]

all sizes are available in dev tools:

We only need to know which ones we need to support and which we don't, because even with the old code because of the threshold, we ignore some devices like ipad which is a mobile but sizes are like the desktop.

[Khadreal]

@wordpressfan @Mai-Saad To know exactly what to do, we need to pick a phone model, get its online specs (CSS pixel and screen size) from the website I shared above for instance. Then, check the returned value for this model in innerWidth.

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 ?

[MathieuLamiot]

The specification is always the same: for mobile devices, anything bigger than 393x830 must be discarded. It means a Samsung Galaxy s8+ of 360x740 must be accepted. the issue reported by Mai is that it is not accepted according to her test. However, I can't reproduce. I'm started to think the develop branch is already correct 🤔 I'll try on e2e @Mai-Saad

Enregistrement de l’écran 2024-05-14 à 12.34.42

[Mai-Saad]

@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)

[MathieuLamiot]

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.

https://github.com/wp-media/wp-rocket/assets/15233030/e9dd0930-8d7f-4dc6-8015-1ab6f8e87c4c

This happens even without WP Rocket installed.

[MathieuLamiot]

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

[MathieuLamiot]

I think I got it. When loading most WordPress sites, I can see this in the

: <meta name="viewport" content="width=device-width, initial-scale=1.0">

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.

[MathieuLamiot]

Closing the PR for now as we don't want to change anything compared to the current behavior.