core(fr): preserve scroll position in gatherers

[adamraine]

This functionality was never brought from legacy navs into FR so we should be consistent. Also, preserving the scroll position will at least be less destructive during flows (#14336).

Additionally, we don't need to restore the scroll position after every gatherer, only tap targets and a11y. I've noticed some issues when we try to restore the scroll position immediately after the bfcache gatherer in legacy mode.

Moving the scroll position restoration inside the tap targets and a11y gatherers achieves both of these goals.

[paulirish]

Additionally, we don't need to restore the scroll position after every gatherer, only tap targets and a11y

nice, yeah i like that.

[connorjclark]

The only problem with this approach currently is that if the gatherer code errors, the position won't be restored. Previously it would have been. Certainly there are times when we've seen Axe throw an error, so we should account for this.

[brendankenny]

FWIW that was added in #8625 so gatherers didn't have to worry about previous gatherers cleaning up after themselves and we wouldn't have to worry about remembering to make gatherers clean up after themselves.

At first glance restoring scroll position after a gatherer seems like a pretty lightweight cleanup step. If the issue is that any evaluateAsync() after the bfcache gatherer runs will throw, that seems like it's going to come up soon or later regardless and it's worth looking into. I see the session error in the logs, I assume because of the navigation. Why don't timespans with (cross-origin?) navigations run into the same problem?

[adamraine]

FWIW that was added in #8625 so gatherers didn't have to worry about previous gatherers cleaning up after themselves and we wouldn't have to worry about remembering to make gatherers clean up after themselves.

Personally I think it's a good exercise to have gatherers clean up after themselves, especially if we're trying to be mindful of how a gatherer affects page state during a flow. #8625 was before my time and I only found out about it because I was investigating the evaluateAsync errors.

At first glance restoring scroll position after a gatherer seems like a pretty lightweight cleanup step. If the issue is that any evaluateAsync() after the bfcache gatherer runs will throw, that seems like it's going to come up soon or later regardless and it's worth looking into. I see the session error in the logs, I assume because of the navigation. Why don't timespans with (cross-origin?) navigations run into the same problem?

This is a good question. My best guess is that we run into errors when evaluateAsync is called immediately after a navigation. The errors seem to pup up when we call Page.getResourceTree. This might be worth opening a separate issue for.

.