Adjust the LRC beacon feature to requirements and by relying on PHP for depth & element identifications

[MathieuLamiot]

Context As spotted by @wordpressfan, the current JS implementation of the LRC feature is quite complex with many recurring calls to explore depth of elements. However, part of the exploration logic is handled PHP side and it would be best to keep it there to simplify maintenance: PHP already handles identifying the LRC candidates based on element type and depth.

Also, a discrepancy in the currently implemented behavior and the expected one by @wp-media/product has been identified and discussed here: https://wp-media.slack.com/archives/CUT7FLHF1/p1724431781878999?thread_ts=1724430625.686259&cid=CUT7FLHF1 The changes required to adapt the behavior should enable to simplify the JS logic on depth and parent/child.

Expected behavior

• The beacon should not try to identify LRC candidates themselves but only rely on PHP by working on elements that have the hash added to them.
• An element must be in the result list of results only if: it is below the fold + threshold, and if none of its parents are on the list.

Acceptance Criteria

• The beacon script does not receive the list of eligible elements anymore.
• The beacon script does not measure depth anymore.
• When increasing the depth of eligible elements PHP-side (through the related filter, yet to be implemented when writing the issue, or manually), the beacon script must consider the elements with the increased depth too.
• In a case where depth is set very high (let's say 10), and with the following page:

  --------- BEGINNING OF PAGE ----------
  SECTION A
  SECTION B
  SECTION C
  --------- END OF VIEWPORT ------------
  SECTION D
       SECTION E
  --------- THRESHOLD
       SECTION F
            SECTION L
  SECTION G
       SECTION H
            SECTION K
       SECTION I
  SECTION J
  --------- END OF PAGE --------------------

The following section hashes must be in the results, and only them: F, G, J

[MathieuLamiot]

Early discussion to maybe consider at grooming: https://wp-media.slack.com/archives/CUT7FLHF1/p1724430625686259

[DahmaniAdame]

All what you shared @MathieuLamiot is what is expected.

For items to be included, the logic is as follow:

• If the parent is already added, there is no need to check or add its children in that case.
• If the parent is not included (above the threshold), children would be checked.

That same logic applies to children as well. Meaning, if a child is added, there is no need to check or add its children, and so on.

Here is example with the expected behavior for each item:

--------- BEGINNING OF PAGE/VIEWPORT ----------
BODY
     TAG [hash=a] ❌ (above threshold) [⚙️ check children]
          TAG [hash=b] ❌ (above threshold)
--------- END OF VIEWPORT (for reference, shouldn't affect how the check is done as it's based on the threshold = distance of the element from the top of the page)
          TAG [hash=c] ❌ (above  threshold) [⚙️ check children]
               TAG [hash=d] ❌ (above threshold)
--------- END OF THRESHOLD
                    TAG [hash=e] ✔️ (below threshold)
               TAG [hash=f] ✔️ (below threshold)
          TAG [hash=g] ✔️ (below threshold) [⚙️ skip checking children]
               TAG [hash=h] ❌ (parent already added)
     TAG [hash=i] ✔️ (below threshold) [⚙️ skip checking children]
          TAG [hash=j] ❌ (parent already added)
               TAG [hash=k] ❌ (parent already added)
                    TAG [hash=l] ❌ (parent already added)
               TAG [hash=m] ❌ (parent already added)
     TAG [hash=n] ✔️ (below threshold) [⚙️ skip checking children]
          TAG [hash=o] ❌ (parent already added)
               TAG [hash=p] ❌ (parent already added)
     TAG [hash=q] ✔️ (below threshold)
--------- END OF PAGE --------------------

e, f, g, i, n, q are the only items to be included.

Regarding the threshold, I read you mentioning below the fold + threshold.

To avoid any confusion, the check is done from the top of page. So, technically, the threshold includes the viewport.

Let us know if you need further clarification or AC.

cc @piotrbak

[jeawhanlee]

Scope a solution ✅

I see 2 sides to update, both JS & PHP

JS

• Update the _getElementDistance method to return just the distance from the top of the viewport to the top of the element which would result in returning just Math.max(0, rect.top + scrollTop).
• Starting with the depth, conditionally calculate it when debug is enabled (this.config.debug) so as to make debug logs readable in console.
• Delete these lines
• Update the condition here & here to become: this._getElementDistance(element.parentElement) < this.config.lrc_threshold && distance > this.config.lrc_threshold
• Update the Log message here as well.

In PHP WP_Rocket\Engine\Optimization\LazyRenderContent\Frontend\Controller::add_custom_data

• Return only [data-rocket-location-hash] in the elements array that way we only target processed elements by PHP.

Estimate the effort ✅

[S]

[MathieuLamiot]

@jeawhanlee I just have a doubt about this condition:

this._getElementDistance(element.parentElement) === 0 && distance > this.config.lrc_threshold

I understand the following:

• _getElementDistance returns the distance of the element from the fold (or 0 if above the fold).
• this._getElementDistance(element.parentElement) === 0 means the parent is above the fold. We should be checking if the parent is above the lrc_threshold instead.
• distance > this.config.lrc_threshold means that the current element is further away than lrc_threshold compared to the fold. This is wrong because we should compare lrc_threshold to the distance to the top of the page: As @DahmaniAdame mentioned, lrc_threshold is the distance from the top of the page at which elements start being eligible.

Am I missing something?

If I am correct, then I suggest:

• Modify _getElementDistance so that it returns the distance to the top of the page, not to the fold. So it should return return Math.max(0, rect.top + scrollTop)
• Change the condition of eligibility of elements to:

this._getElementDistance(element.parentElement) < this.config.lrc_threshold && distance >= this.config.lrc_threshold

[jeawhanlee]

@MathieuLamiot What that condition means is lazy render only all sub-parent elements that has their parent element at distance 0 and their own distance greater than the threshold, that way we only get the parent elements and not the children.

You can validate with me using this template here: https://user-f89ec7c1-e805-4205-806b-9d7fe56d9853-45htvp2guq-uc.a.run.app/?p=de8c45bc6c

The debug has this:

What I think we need to enhance is factoring in parent elements that might be less than the threshold but not 0 so I think we can change this: this._getElementDistance(element.parentElement) === 0 && distance > this.config.lrc_threshold to become this: this._getElementDistance(element.parentElement) < this.config.lrc_threshold && distance > this.config.lrc_threshold

[MathieuLamiot]

Following our discussion @jeawhanlee:

• we agree on the condition this._getElementDistance(element.parentElement) < this.config.lrc_threshold && distance >= this.config.lrc_threshold
• There might be something wrong with _getElementDistance. I think (and confirmed by ChatGPT hehe) that it currently returns the distance between the top of the element and the bottom of the viewport. But the threshold is actually the distance between the element and the beginning of the page according to @DahmaniAdame comment. I'll let you confirm, but I think we should also change _getElementDistance to return Math.max(0, rect.top + scrollTop)

[MathieuLamiot]

    const rect = element.getBoundingClientRect();
    const scrollTop = window.pageYOffset || document.documentElement.scrollTop;
    let distanceFromViewportTop = rect.top + scrollTop - viewportHeight;

    // Ensure distance is non-negative
    distanceFromViewportTop = Math.max(0, distanceFromViewportTop);

    // Apply styling based on distance
    let style = '';
    if (distanceFromViewportTop > 1800) {
        style = 'color: green;';
    } else if (distanceFromViewportTop === 0) {
        style = 'color: red;';
    }

I see that the formula + comparison to the threshold comes from @DahmaniAdame's prototype. Maybe @DahmaniAdame you can confirm our discussion and this is creating the confusion:

• You mentionned that the threshold should be "from the top of the page":

To avoid any confusion, the check is done from the top of page. So, technically, the threshold includes the viewport.

• But the formula used in the EPIC card seems to compare the threshold value to the distance between the element and the bottom of the viewport.

[jeawhanlee]

@MathieuLamiot Grooming updated in respect to our discussions.

[Mai-Saad]

Note: to be retested with the fix here https://new.rocketlabsqa.ovh/lrc_similive/ , here we have 2 hashes in threshold and not added to DB bde801b81055e486a3cf54938b78ca40and 8c2d3df5df458ee91f4369f5ed9543f0