Improve speculation rules handling based on element visibility

[giorgiopellegrino]

Pull Request Description:
This pull request addresses the issue raised in #442 by enhancing how speculation rules are managed based on the visibility of DOM elements.

Specifically, speculation rules are now added only when the target element is within the viewport and removed when it exits the viewport. This change enables developers to safely use quicklink.listen(el, { prerender: true, eagerness: 'immediate' }) on mobile devices without the risk of hitting the browser-imposed limit of 10 prerendered pages.

This implementation improves resource management and ensures a more efficient and predictable prerendering behavior.

Thanks,

Giorgio Pellegrino.

cc @gilbertococchi

[gilbertococchi]

This is great Giorgio!

I've run some tests and it looks like a great start!

Would you be open to have a testing HTML page added to verify the behaviour and also add some documentation changes that explains what this combination will work on QuickLink?

[XhmikosR]

CI fails, please make sure tests are valid.

[gilbertococchi]

TODO in order to complete merge this PR:

• Document the behaviour in the Documentation docs
• Fix failing test
• Removing eagerness=immediate condition on unobserve call.

[gilbertococchi]

Thanks Giorgio, all the changes here looks good now.

[gilbertococchi]

Thanks @giorgiopellegrino for adding the documentation changes, can you please add them as part of this PR?

Context: giorgiopellegrino@f522eef

[addyosmani]

This is looking close to ready to merge! @giorgiopellegrino can we update it to be merge-friendly with the main branch and double check CI is passing?

[giorgiopellegrino]

This is looking close to ready to merge! @giorgiopellegrino can we update it to be merge-friendly with the main branch and double check CI is passing?

Hi @addyosmani ,
I've merged from the updated main branch and can confirm that it passes the local tests.

[westonruter]

I wonder why this is necessary? Is it to create a copy to so that if the underlying object is modified during some async iteration to prevent unexpected behavior? I don't see this as being the case, since this is not an async function and below it just loops over the urls to add each to the toPrerender Set.

I think this can just be removed.

Suggested change

urls = [].concat(urls);

[giorgiopellegrino]

Hello @westonruter ,
this line of code was already present. In my opinion, it is used to ensure that urls is treated as an array of strings, since the prerender function accepts either a single string or an array of strings as its argument.

[gilbertococchi]

Hi @westonruter, this line was already present in the existing Speculation Rules implementation, so not really related to this PR.

It was done this way to ensure that URLs would be treated as array of strings.

@giorgiopellegrino to follow up in case an action is needed

[westonruter]

I wonder if immediate is too eager? Why not eager? It's true that today they behave the same, but the intention is that eager can could potentially be more nuanced in the future, per @tunetheweb:

eager: This currently behaves identically to the immediate setting, but in future, we are looking to place this somewhere between immediate and moderate.

Since there could be a lot of links in the viewport without any signal about whether they are going to be navigated to (other than them being in the viewport), it would seem like immediate is too aggressive.

[tunetheweb]

eager is likely to be replaced with a short (or even 0) hover time—basically moderate with minimal delay. So it would still require a user interaction. I'd imagine if/when that happens we might reduce the limit to be 2 similar to moderate and conservative.

The intent for this logic is particularly intended for mobile, where there is no hover, so immediate seems like the right solution.

[giorgiopellegrino]

Hi,
would you recommend keeping the default argument as immediate, or should it be set to eager?