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

3.17 - Consider the viewport while calculating _getElementDistance #6933

Closed
DahmaniAdame opened this issue Aug 30, 2024 · 6 comments · Fixed by #6939 or wp-media/rocket-scripts#28
Closed
Assignees
Milestone

Comments

@DahmaniAdame
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Consider the viewport size when calculating element's distance to the viewport to avoid misuse of filtering the threshold.

    _getElementDistance(element) {
      const rect = element.getBoundingClientRect();
      const scrollTop = window.pageYOffset || document.documentElement.scrollTop;
      return Math.max(0, rect.top + scrollTop - (window.innerHeight || document.documentElement.clientHeight));
    }

Describe the solution you'd like
N/A

Describe alternatives you've considered
N/A

Additional context
Related to PR - #6929

@piotrbak piotrbak added this to the 3.17 milestone Aug 30, 2024
@wordpressfan wordpressfan self-assigned this Sep 3, 2024
@wordpressfan
Copy link
Contributor

@DahmaniAdame So here we will keep the threshold to be 1800px after the viewport? I also need to make sure that this value will be exactly the same in mobile and desktop or not.

I have a scenario that I don't know if it'll affect the approach here or not,
Think of a page without lrc data in the database and a visitor opened that page from a tablet so it'll calculate the lrc based on that screen then save it in database as mobile version of lrc and then a small mobile device visited the page, will this affect anything here to have the lrc of tablet to be used on mobile?

@DahmaniAdame
Copy link
Contributor Author

DahmaniAdame commented Sep 3, 2024

Yes, it will be 1800px.
For now, the value is the same for both mobile and desktop (content-visibility is triggered around that point for both).
The scenario you shared is possible and acceptable for now. If it proves to be an issue (if it is, it would be for any feature dependent on device size, like RUCSS and OCI), we can consider adding a 3rd cache specific for tablets to increase accuracy.

@piotrbak
Copy link
Contributor

piotrbak commented Sep 3, 2024

The risk is the same also for OCI where it's probably more negative impact

@wordpressfan
Copy link
Contributor

Awesome, many thanks @DahmaniAdame @piotrbak

@wordpressfan
Copy link
Contributor

@wp-media/product
When we have a parent element and its top is 1800px and that's exactly the threshold value should we consider saving this parent hashes or only consider children?

@DahmaniAdame
Copy link
Contributor Author

DahmaniAdame commented Sep 3, 2024

When we have a parent element and its top is 1800px and that's exactly the threshold value should we consider saving this parent hashes or only consider children?

@wordpressfan it will be quite rare to have something in the precise point of the threshold. But if it does, we should collect only the parent's hash for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants