Disconnect performance observer when page is already hidden

[monis0395]

Added a check in onHidden.ts, to check if page is already hidden when onHidden() is called. This would allow listeners like FID, LCP to disconnect performance observer whenever the page is hidden

[monis0395]

This will resolve part 1 of #193

[vanderhoop]

Any movement on this? It would seem this change would avoid unrepresentative metrics being reported.

[monis0395]

@philipwalton can u please review this PR ?

if @vanderhoop is talking about the case where page was already hidden when the module loaded then become visible getLCP would be reporting false metrics when reportAllChanges is true?

This PR:#196 or PR:#197 should fix that!

[philipwalton]

Adding this will cause problem for other functions that use onHidden. We do not want to call the callback immediately—only when the page is actually hidden.

I see that your changes to the visibilityWatcher.ts file account for calling it right away, but other modules import this function as well, and it won't work in many of those cases.

[philipwalton]

Sorry for the late reply, @monis0395. I took a look at your proposed changes, and (unless I'm missing something), I don't think this addresses the original issue in #193.

The point you brought up there was that Performance Observer objects might be created in situation where they don't have to be (because the page is already hidden). In those cases I think the best solution is to not create them in the first place rather than creating them and immediately disconnecting them.

[philipwalton]

Any movement on this? It would seem this change would avoid unrepresentative metrics being reported.

@vanderhoop this PR and the linked issue do not change what data the library reports, it only changes whether or not a Performance Observer object is created. Please file an issue if you're seeing cases where LCP is reported for pages loaded in the background.

[vanderhoop]

@vanderhoop this PR and the linked issue do not change what data the library reports, it only changes whether or not a Performance Observer object is created. Please file an issue if you're seeing cases where LCP is reported for pages loaded in the background.

Hey @philipwalton, thanks for the quick response 🙏. My coworker, @dvndrsn, has cooked up a great reproduction video for the odd behavior we're seeing for LCP reporting for tabs that have been backgrounded. I'll work with him to scrub it and submit an issue.

[monis0395]

The point you brought up there was that Performance Observer objects might be created in situation where they don't have to be (because the page is already hidden).

Yes! You are correct about this

In those cases I think the best solution is to not create them in the first place rather than creating them and immediately disconnecting them.

I am immediately disconnecting Performance Observer(po) because onBFCacheRestore is only attached when po is present. Thus I thought of immediately disconnecting them. I am not fully aware when should we attach onBFCacheRestore

If u have any approaches, I'll be glad to implement them

PS: #197 (comment) this was quite informative.
I did not know that when the document visibility changes from hidden -> visible, no LCP value would be set on the metric

PPS: sorry for the late reply

[philipwalton]

Closing as this is no longer doable since the code has changed since this PR was filed.