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

core(fr): preserve scroll position in gatherers #14660

Merged
merged 6 commits into from
Jan 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import {pageFunctions} from '../../lib/page-functions.js';
*/
/* c8 ignore start */
async function runA11yChecks() {
const originalScrollPosition = {
x: window.scrollX,
y: window.scrollY,
};

/** @type {import('axe-core/axe')} */
// @ts-expect-error - axe defined by axeLibSource
const axe = window.axe;
Expand Down Expand Up @@ -72,13 +77,17 @@ async function runA11yChecks() {
// are relative to the top of the page
document.documentElement.scrollTop = 0;

return {
const result = {
violations: axeResults.violations.map(createAxeRuleResultArtifact),
incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact),
notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable!
passes: axeResults.passes.map(result => ({id: result.id})),
version: axeResults.testEngine.version,
};

window.scrollTo(originalScrollPosition.x, originalScrollPosition.y);

return result;
}

/**
Expand Down
7 changes: 7 additions & 0 deletions core/gather/gatherers/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ function gatherTapTargets(tapTargetsSelector, className) {
/** @type {LH.Artifacts.TapTarget[]} */
const targets = [];

const originalScrollPosition = {
x: window.scrollX,
y: window.scrollY,
};

// Capture element positions relative to the top of the page
window.scrollTo(0, 0);

Expand Down Expand Up @@ -279,6 +284,8 @@ function gatherTapTargets(tapTargetsSelector, className) {

reenableFixedAndStickyElementPointerEvents();

window.scrollTo(originalScrollPosition.x, originalScrollPosition.y);

return targets;
}
/* c8 ignore stop */
Expand Down
17 changes: 0 additions & 17 deletions core/legacy/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,23 +405,6 @@ class Driver {
return fetchResponseBodyFromCache(this.defaultSession, requestId, timeout);
}

/**
* @param {{x: number, y: number}} position
* @return {Promise<void>}
*/
scrollTo(position) {
const scrollExpression = `window.scrollTo(${position.x}, ${position.y})`;
return this.executionContext.evaluateAsync(scrollExpression, {useIsolation: true});
}

/**
* @return {Promise<{x: number, y: number}>}
*/
getScrollPosition() {
return this.executionContext.evaluateAsync(`({x: window.scrollX, y: window.scrollY})`,
{useIsolation: true});
}

/**
* @param {{additionalTraceCategories?: string|null}=} settings
* @return {Promise<void>}
Expand Down
6 changes: 0 additions & 6 deletions core/legacy/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,17 +313,12 @@ class GatherRunner {
* @return {Promise<void>}
*/
static async afterPass(passContext, loadData, gathererResults) {
const driver = passContext.driver;
const config = passContext.passConfig;
const gatherers = config.gatherers;

const apStatus = {msg: `Running afterPass methods`, id: `lh:gather:afterPass`};
log.time(apStatus, 'verbose');

// Some gatherers scroll the page which can cause unexpected results for other gatherers.
// We reset the scroll position in between each gatherer.
const scrollPosition = await driver.getScrollPosition();

for (const gathererDefn of gatherers) {
const gatherer = gathererDefn.instance;
const status = {
Expand All @@ -339,7 +334,6 @@ class GatherRunner {
gathererResult.push(artifactPromise);
gathererResults[gatherer.name] = gathererResult;
await artifactPromise.catch(() => {});
await driver.scrollTo(scrollPosition);
log.timeEnd(status);
}
log.timeEnd(apStatus);
Expand Down
10 changes: 0 additions & 10 deletions core/test/legacy/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import {fnAny, readJson} from '../../test-utils.js';
* @param {{protocolGetVersionResponse: LH.CrdpCommands['Browser.getVersion']['returnType']}} param0
*/
function makeFakeDriver({protocolGetVersionResponse}) {
let scrollPosition = {x: 0, y: 0};

return {
get fetcher() {
return {};
Expand Down Expand Up @@ -56,14 +54,6 @@ function makeFakeDriver({protocolGetVersionResponse}) {
return Promise.resolve();
},
},
/** @param {{x: number, y: number}} position */
scrollTo(position) {
scrollPosition = position;
return Promise.resolve();
},
getScrollPosition() {
return Promise.resolve(scrollPosition);
},
beginTrace() {
return Promise.resolve();
},
Expand Down
34 changes: 0 additions & 34 deletions core/test/legacy/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -687,40 +687,6 @@ describe('GatherRunner', function() {
});
});

it('resets scroll position between every gatherer', async () => {
class ScrollMcScrollyGatherer extends TestGatherer {
/** @param {{driver: Driver}} context */
afterPass(context) {
context.driver.scrollTo({x: 1000, y: 1000});
}
}

const url = 'https://example.com';
const driver = Object.assign({}, fakeDriver);
const scrollToSpy = jestMock.spyOn(driver, 'scrollTo');

const passConfig = {
recordTrace: true,
gatherers: [
{instance: new ScrollMcScrollyGatherer()},
{instance: new TestGatherer()},
],
};

/** @type {any} Using Test-only gatherer. */
const gathererResults = {
TestGatherer: [],
};
const passContext = {url, driver, passConfig, computedCache: new Map()};
await GatherRunner.afterPass(passContext, {}, gathererResults);
// One time for the afterPass of ScrollMcScrolly, two times for the resets of the two gatherers.
expect(scrollToSpy.mock.calls).toEqual([
[{x: 1000, y: 1000}],
[{x: 0, y: 0}],
[{x: 0, y: 0}],
]);
});

it('does as many passes as are required', async () => {
const t1 = new TestGatherer();
const t2 = new TestGatherer();
Expand Down