-
Notifications
You must be signed in to change notification settings - Fork 423
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
Update onINP to also observe first-input entries #232
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -119,6 +119,26 @@ export const onINP = (onReport: ReportCallback, opts?: ReportOpts) => { | |
if (entry.interactionId) { | ||
processEntry(entry); | ||
} | ||
|
||
// Entries of type `first-input` don't currently have an `interactionId`, | ||
// so to consider them in INP we have to first check that an existing | ||
// entry doesn't match the `duration` and `startTime`. | ||
// 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. | ||
// TODO(philipwalton): remove once crbug.com/1325826 is fixed. | ||
if (entry.entryType === 'first-input') { | ||
const noMatchingEntry = !longestInteractionList.some((interaction) => { | ||
return interaction.entries.some((prevEntry) => { | ||
return entry.duration === prevEntry.duration && | ||
entry.startTime === prevEntry.startTime; | ||
}); | ||
}); | ||
if (noMatchingEntry) { | ||
processEntry(entry); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if:
I think it will be treated as 2 different interactions here, also affecting p98. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think so. My hope is we can remove this logic entirely once |
||
} | ||
} | ||
}); | ||
|
||
const inp = estimateP98LongestInteraction(); | ||
|
@@ -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}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this seems better :) |
||
|
||
onHidden(() => { | ||
handleEntries(po.takeRecords()); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this related to FID entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, not sure why but I started noticing lots of cases where |
||
})); | ||
|
||
// Test sending the metric to an analytics endpoint. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theevent
entry processed before thefirst-input
entry.