Update onINP to also observe first-input entries #232
Conversation
| // Note that this logic assumes that `event` entries are dispatched | ||
| // before `first-input` entries. This is true in Chrome but it is not | ||
| // true in Firefox; however, Firefox doesn't support interactionId, so | ||
| // it's not an issue at the moment. |
There was a problem hiding this comment.
I was surprised to learn this, since I think we fire FID first.
However, I guess you mean that the order of Entries in the PO callbacks, when there are multiple observe() calls of different types... This I haven't tested and I'm not sure if order is guaranteed or arbitrary right now.
There was a problem hiding this comment.
Yeah, I only tested this in Chrome on Mac (desktop), but no matter what I did, which order I called observe() in, or which event I used, I always got the event entry processed before the first-input entry.
| }); | ||
| }); | ||
| if (noMatchingEntry) { | ||
| processEntry(entry); |
There was a problem hiding this comment.
What happens if:
- FID is pointerdown, but we do not get a pointerdown event timing entry
- Then we do get an pointerup event timing entry (for the same interaction)
I think it will be treated as 2 different interactions here, also affecting p98.
There was a problem hiding this comment.
Yeah, I think so. My hope is we can remove this logic entirely once first-input entries have an interactionId property. My goal here was to minimize these types of occurrences .
| @@ -143,6 +163,10 @@ export const onINP = (onReport: ReportCallback, opts?: ReportOpts) => { | |||
| report = bindReporter(onReport, metric, opts.reportAllChanges); | |||
|
|
|||
| if (po) { | |||
| // Also observe entries of type `first-input`. This is useful in cases | |||
| // where the first interaction is less than the `durationThreshold`. | |||
| po.observe({type: 'first-input', buffered: true}); | |||
| @@ -89,7 +89,7 @@ | |||
| inp.entries = inp.entries.map((e) => ({ | |||
| ...e.toJSON(), | |||
| interactionId: e.interactionId, | |||
| target: e.target.nodeName.toLowerCase(), | |||
| target: e.target?.nodeName.toLowerCase(), | |||
There was a problem hiding this comment.
Yeah, not sure why but I started noticing lots of cases where target was undefined on first-input entries. I didn't look much further into repo-ing why.
Fixes #228 by having the observer created in
onINP()also observefirst-inputentries (in addition toevent)—given that they are both instances ofPerformanceEventTiming.This PR also includes logic to remove duplicate interactions by checking to see if a
first-inputandevententry both have the samestartTimeanddurationvalues. This logic is needed until crbug/1325826 is fixed in Chrome, which adds support for theinteractionIdproperty onfirst-inputentries./cc @mmocny