Skip to content
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

Mobile manual visit isnot added to ATF table when meta viewport header is missing #6628

Closed
Mai-Saad opened this issue May 10, 2024 · 3 comments
Labels
lcp needs: grooming priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior

Comments

@Mai-Saad
Copy link

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version => 3.16
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
Manually visiting URL, won't add the URL to the database although the screen size is valid

To Reproduce
Steps to reproduce the behavior:

  1. 3.16 installed and activated
  2. Visit a page in mobile view i.e Samsung Galaxy s8+ (360 x 740) https://e2e.rocketlabsqa.ovh/lcp_bg_samestyle_template/
  3. Check ATF table => no entry is added
  4. Visit the same page in desktop view then check ATF => desktop entry is added

Expected behavior
Mobile visit adds an entry in the ATF table as the separate mobile cache is enabled by default on 3.16 fresh install

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

Acceptance Criteria (for WP Media team use only)
Clear instructions for developers, to be added before the grooming

  • Manual visit adds an entry to ATF if the size is valid for both desktop and mobile (as long as a separate cache is on)
  • No entry is added to the database with an invalid screen of both desktop/mobile
  • If a separate mobile cache was disabled using a filter then no mobile entry is added by visiting mobile
@Mai-Saad Mai-Saad added type: bug Indicates an unexpected problem or unintended behavior priority: medium Issues which are important, but no one will go out of business. severity: major Feature is not working as expected and no work around available lcp labels May 10, 2024
@piotrbak piotrbak added priority: high Issues which should be resolved as quickly as possible needs: grooming and removed priority: medium Issues which are important, but no one will go out of business. labels May 10, 2024
@wordpressfan
Copy link
Contributor

I think that's because the device pixel ratio window.devicePixelRatio
To validate that this is the root cause, I changed those two lines of code:

const screenWidth = window.innerWidth || document.documentElement.clientWidth;
const screenHeight= window.innerHeight || document.documentElement.clientHeight;

to be:

const screenWidth = ( window.innerWidth || document.documentElement.clientWidth ) / ( document.devicePixelRatio || 1 );
const screenHeight= ( window.innerHeight || document.documentElement.clientHeight ) / ( document.devicePixelRatio || 1 );

and the data was sent properly, the thing that I want to validate next is that we need to do the same with each use of window.innerWidth

@MathieuLamiot MathieuLamiot changed the title Mobile manual visit isnot added to ATF table Mobile manual visit isnot added to ATF table when meta viewport header is missing May 14, 2024
@MathieuLamiot
Copy link
Contributor

The issue is related to not responsive templates that don't use the viewport meta header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lcp needs: grooming priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants