Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

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

@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

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

EDIT: very quick test with commit c35559c 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?

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings    Change summary preferences

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

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.

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

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

@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

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.

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

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

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

This 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 :
<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.

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