core(cls): ignore had_recent_input by timing window

[adamraine]

Proposal that helps with #14396 for our use cases. We should wait on a chromium fix before closing though.

See my comments for #14396 for the reasoning.

[connorjclark]

Perhaps we should consider the alternative approach mentioned in https://bugs.chromium.org/p/chromium/issues/detail?id=1094974#c19 , which both annie and yoav vouched for–do our emulation resizing and wait 500ms before navigating to the page.

[paulirish]

IMO this a chromium bug. It shouldn't treat a window resize in a previous navigation as "recent input".

https://bugs.chromium.org/p/chromium/issues/detail?id=1302667#c21

[adamraine]

IMO this a chromium bug. It shouldn't treat a window resize in a previous navigation as "recent input".

I agree this should no longer close #14396, but it's still better that what we had before.

[connorjclark]

Can you confirm the delta in the sample json tests are expected?

[connorjclark]

Although this PR is strictly better, we want to rid ourselves of our hacky "ignore had_recent_input.... sometimes" approach entirely. And we are hesitant to do the 500ms-waiting approach before about:blank nav.

We need to address this as a chromium bug (ref).

Maybe we can determine when the cutoff for CLS events by looking at the navigation (or timespan equivalent) timestamp?

[adamraine]

Can you confirm the delta in the sample json tests are expected?

The delta in the JSON is not expected. I'm going to ping upstream for help.

[adamraine]

I did some investigation. Instead of looking at LS events within 500ms of the time origin, we should look at LS events within 500ms of the first "viewport" event.

Even though emulation is started before navigating, Chrome still triggers a viewport update event after the navigation starts which gets treated as an interaction for LS had_recent_input purposes:
https://bugs.chromium.org/p/chromium/issues/detail?id=1302667#c19

[brendankenny]

Why keep this? If we're confident in the viewport event it seems like keeping our old heuristic just makes things more complicated.

[adamraine]

This is a good point, I suppose the reason is because I'm not confident. Having a surefire exit from our special CSS handling once we see had_recent_input: false could be good logic to keep defensively.

[brendankenny]

Yeah, that's probably fine, there's not a real downside to keeping. I was really only thinking about it because tests like

lighthouse/core/test/computed/metrics/cumulative-layout-shift-test.js

Lines 130 to 145 in 90797a1

	it('should not count later shift events if input it true', async () => {
	const context = {computedCache: new Map()};
	const trace = makeTrace([
	{score: 1, ts: 1, had_recent_input: true},
	{score: 1, ts: 2, had_recent_input: false},
	{score: 1, ts: 3, had_recent_input: false},
	{score: 1, ts: 4, had_recent_input: true},
	{score: 1, ts: 5, had_recent_input: true},
	]);
	const result = await CumulativeLayoutShift.request(trace, context);
	expect(result).toEqual({
	cumulativeLayoutShift: 3,
	cumulativeLayoutShiftMainFrame: 3,
	totalCumulativeLayoutShift: 3,
	});
	});

seem completely unrealistic now, and hopefully our future selves won't consider them as evidence of essential functionality that needs to be maintained if there are future CLS event changes. Maybe finding this discussion will be enough :)

nvm