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(gather-runner): always reset scroll position #8625

Merged
merged 7 commits into from
May 25, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
1 change: 0 additions & 1 deletion lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ const defaultConfig = {
'seo/embedded-content',
'seo/robots-txt',
'seo/tap-targets',
// Always run axe last because it scrolls the page down to the bottom
'accessibility',
],
},
Expand Down
35 changes: 34 additions & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ class Driver {
this._handleReceivedMessageFromTarget(event, []).catch(this._handleEventError);
});

this.on('Runtime.executionContextDestroyed', event => {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
if (event.executionContextId === this._isolatedExecutionContextId) {
this._clearIsolatedContextId();
}
});

this.on('Page.frameNavigated', this._clearIsolatedContextId.bind(this));
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved

connection.on('protocolevent', this._handleProtocolEvent.bind(this));

/**
Expand Down Expand Up @@ -464,7 +472,16 @@ class Driver {
*/
async evaluateAsync(expression, options = {}) {
const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined;
return this._evaluateInContext(expression, contextId);
return this._evaluateInContext(expression, contextId).catch(async err => {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// If we were using isolation and the context disappeared on us, retry one more time.
if (contextId && err.message.includes('Cannot find context')) {
this._clearIsolatedContextId();
const freshContextId = await this._getOrCreateIsolatedContextId();
return this._evaluateInContext(expression, freshContextId);
}

throw err;
});
}

/**
Expand Down Expand Up @@ -1324,6 +1341,22 @@ class Driver {
return flattenedDocument.nodes ? flattenedDocument.nodes : [];
}

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

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

/**
* @param {{additionalTraceCategories?: string|null}=} settings
* @return {Promise<void>}
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,10 @@ class GatherRunner {
await driver.setThrottling(passContext.settings, {useThrottling: false});
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 = pageLoadError ? null : await driver.getScrollPosition();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pageLoadError control flow is seriously the worst :S


for (const gathererDefn of gatherers) {
const gatherer = gathererDefn.instance;
const status = {
Expand All @@ -340,6 +344,7 @@ class GatherRunner {
gathererResult.push(artifactPromise);
gathererResults[gatherer.name] = gathererResult;
await artifactPromise.catch(() => {});
if (scrollPosition) await driver.scrollTo(scrollPosition);
log.timeEnd(status);
}
log.timeEnd(apStatus);
Expand Down
13 changes: 13 additions & 0 deletions lighthouse-core/test/gather/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,19 @@ describe('.evaluateAsync', () => {
expect.anything()
);
});

it('recovers from isolation failures', async () => {
connectionStub.sendCommand = createMockSendCommandFn()
.mockResponse('Page.getResourceTree', {frameTree: {frame: {id: 1337}}})
.mockResponse('Page.createIsolatedWorld', {executionContextId: 9001})
.mockResponse('Runtime.evaluate', Promise.reject(new Error('Cannot find context')))
.mockResponse('Page.getResourceTree', {frameTree: {frame: {id: 1337}}})
.mockResponse('Page.createIsolatedWorld', {executionContextId: 9002})
.mockResponse('Runtime.evaluate', {result: {value: 2}});

const value = await driver.evaluateAsync('1 + 1', {useIsolation: true});
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
expect(value).toEqual(2);
});
});

describe('.sendCommand', () => {
Expand Down
9 changes: 9 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
'use strict';

function makeFakeDriver({protocolGetVersionResponse}) {
let scrollPosition = {x: 0, y: 0};

return {
getBrowserVersion() {
return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71}));
Expand Down Expand Up @@ -57,6 +59,13 @@ function makeFakeDriver({protocolGetVersionResponse}) {
evaluateAsync() {
return Promise.resolve({});
},
scrollTo(position) {
scrollPosition = position;
return Promise.resolve();
},
getScrollPosition() {
return Promise.resolve(scrollPosition);
},
registerPerformanceObserver() {
return Promise.resolve();
},
Expand Down
28 changes: 28 additions & 0 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,34 @@ describe('GatherRunner', function() {
});
});

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

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

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

await GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []});
// 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', () => {
const t1 = new TestGatherer();
const t2 = new TestGatherer();
Expand Down