Skip to content
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

Merged
merged 1 commit into from May 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/onINP.ts
Expand Up @@ -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.
Copy link
Member

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.

Copy link
Member Author

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 the event entry processed before the first-input entry.

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 first-input entries have an interactionId property. My goal here was to minimize these types of occurrences .

}
}
});

const inp = estimateP98LongestInteraction();
Expand All @@ -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});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems better :)


onHidden(() => {
handleEntries(po.takeRecords());

Expand Down
2 changes: 1 addition & 1 deletion test/views/inp.njk
Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to FID entry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}));

// Test sending the metric to an analytics endpoint.
Expand Down