Fix negative inputDelay in INP attribution - #789
Merged
tunetheweb merged 1 commit intoAug 25, 2026
Merged
Conversation
Clamp the frame-wide group.processingStart to the interaction's own startTime so inputDelay can never be reported as negative. group.processingStart is the minimum across every event presented in the frame, which can predate the interaction when a non-interaction handler was still running. Zero-interactionId events still contribute to processingDuration, and the three subparts still sum to metric.value. Also drops the now-redundant first-input checks, since first-input has exposed a non-zero interactionId for some years, and adds unit coverage for both the subparts and the first-input candidate path. Fixes GoogleChrome#788
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #788.
Clamps
group.processingStartto the interaction's ownstartTimeinattributeINP(), as discussed in the issue:This keeps events with
interactionId === 0counted inprocessingDuration, per the subpart definitions you set out — input delay is non-handler code, processing is handler code including zero-interactionIdevents. I verified that directly: with an interaction starting at 1000 and aninteractionId: 0handler running 1050–1090,processingStartstill resolves to 1050, so that handler's time lands inprocessingDurationand the entry stays inprocessedEventEntries. The clamp only discards zero-interactionIdtime that predates the interaction's ownstartTime, which was never part of its latency.In the issue's repro this gives
inputDelay: 0,processingDuration: 75,presentationDelay: 29(was-98 / 173 / 29).Worth noting the clamp can only raise
processingStartas far asfirstEntry.startTime, which is<= startTime + duration, sonextPaintTimecannot move.metric.valueandpresentationDelayare therefore unchanged, and onlyinputDelayandprocessingDurationshift by equal and opposite amounts — the three subparts still sum tometric.value.first-input cleanup
Also drops the now-redundant
|| entry.entryType === 'first-input'clause. I confirmed in Chrome 151 thatfirst-inputexposes a non-zerointeractionId(measured7282and4643in separate runs), and that it is still delivered whendurationThresholdwould filter the correspondingevententry — so the fallback for fast-only interactions is preserved.Two notes on this part:
lib/InteractionManager.ts:118andattribution/onINP.ts:213. I simplified both for consistency; happy to drop the second if you'd rather keep this focused.interactionIdin Chrome. If Firefox or Safari still reportfirst-inputwithinteractionId: 0, the simplification would stop INP being reported at all on pages whose only interactions fall underdurationThreshold. CI covers both browsers, so it should surface here, but flagging it since the failure mode is a missing metric rather than a wrong value.Tests
test/unit/attribution-onINP-test.js(new) covers the subparts:inputDelayscenario — fails onmainwithexpected: 0, actual: -98, passes with the clamp30 / 55 / 25test/unit/InteractionManager-test.jsgains two cases pinning the guard, since there was no existing coverage of thefirst-inputpath (grep first-input test/e2e/onINP-test.jsreturns nothing): afirst-inputentry with aninteractionIdis still treated as an INP candidate, and entries without one are skipped. Both pass under the old and new guard by design — they pin behaviour that must hold either way.These are unit tests rather than e2e deliberately. The bug needs
pointerdown.startTimeto fall after another event'sprocessingStart, and WebDriver dispatches a whole click with a single hardware timestamp — I tried, and the synthesized entries all sharestartTime, so an e2e test can't express it.One thing I have not measured
The
longestScript.subpartboundaries reduce toprocessingStartandprocessingEnd, so the clamp shifts them: a script starting before the interaction now falls in theinput-delaybucket rather thanprocessing-duration, in a bucket whose reported width is 0. I derived that from reading lines 351-358 rather than measuring it, since it needs LoAF entries. Flagging rather than asserting, in case you want it handled differently.Verification
build,lintandformat:checkclean.test:unit26/26. Full--browsers=chromee2e: 5/5 spec files, 153 passing, 4 skipped. Reverting just the clamp leaves the new test failing and everything else green.