Cap INP breakdowns to INP duration #528
Conversation
src/attribution/onINP.ts
Outdated
| // mark event timing duration as that paint. | ||
| // See: https://github.com/GoogleChrome/web-vitals/issues/492 | ||
| // So cap to the INP value. | ||
| const nextPaintTime = Math.min( |
There was a problem hiding this comment.
I'm not sure about this change...
Looking at like 276 above, you are already including startTime + duration in the list of paint time candidates, which means its in the list of MAX and then also in the list of MIN. I think that means its always going to be picked?
(Note: I assume there is no difference between firstEntry.duration and metric.value but there can be)
Second, the comment anove is:
// Since entry durations are rounded to the nearest 8ms, we need to clamp
// the `nextPaintTime` value to be higher than the `processingEnd` or
// end time of any LoAF entry.I think we are specifically looking for cases where startTime + duration is "accidentally" less than one of the alternative paintTimeCandidates (such as processingEnd) when we do rounding.
Now here by using startTime + duration when it is less than the alternatives, we are effectively just undoing that?
Might as well just delete this code instead? Or, selectively do it if its "less than 8ms off" or something?
Finally, also already capped the processingEnd time in this CL, so its also not useful to look at processingEnd as an alternative to paint time any more (it will always be smaller now).
There was a problem hiding this comment.
Now here by using startTime + duration when it is less than the alternatives, we are effectively just undoing that?
Urgh! I realised this earlier, and then completely forgot about it and committed it with further.
Might as well just delete this code instead?
I think we need to do something here, or we'll just move this incorrect breakdown to presentation delay.
Or, selectively do it if its "less than 8ms off" or something?
Yeah think we need something like this. Or if processingEnd is firstEntry.startTime + metric.value, then we can 0 out the presentation time.
There was a problem hiding this comment.
Have another look not if you can @mmocny
| @@ -239,10 +239,16 @@ const getIntersectingLoAFs = ( | |||
|
|
|||
| const attributeINP = (metric: INPMetric): INPMetricWithAttribution => { | |||
| const firstEntry = metric.entries[0]; | |||
| const inpTime = firstEntry.startTime + metric.value; | |||
There was a problem hiding this comment.
Nit: just call it endTime or something
| const group = entryToEntriesGroupMap.get(firstEntry)!; | ||
|
|
||
| const processingStart = firstEntry.processingStart; | ||
| const processingEnd = group.processingEnd; | ||
| // processingEnd can extend beyond duration for modals where we artificially | ||
| // mark event timing duration as that paint. |
There was a problem hiding this comment.
Nit: "as that paint" comment is confusing (there is no actual paint)
Perhaps just "where duration ends at the time modal dialog started (and provided visual feedback)"
| // If processingEnd has been capped to inpTime then presentationDelay is 0 | ||
| // Else use the nextPaintTime | ||
| const presentationDelay = | ||
| processingEnd == inpTime ? 0 : Math.max(nextPaintTime - processingEnd, 0); |
There was a problem hiding this comment.
I think this is a fine temporary solution, but, consider just creating a variable such as "used_fallback_time", set up top when endTime < processingEnd, and re-use that everywhere.
(The goal is to expose a clue to event timing for this value, eventually.)
BTW the case where endTime is rounded to within 8ms of processingEnd is almost always another "used_fallback_time" example (i.e. when we actually dont have a paint)
Fixes #492
Modals (
alert(),confirm(),print()) do give feedback to the user, but also block the main thread so the event processing doesn't end when this shows. However, from an INP point of view, the feedback is given and so the duration is set to that timestamp.I think it should be possible add a test case for this, but wanna agree on the change before going to that effort.