From b2b018dc1a44f7a6309bbc962f4d0d0d2745321e Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 24 May 2019 22:54:40 -0700 Subject: [PATCH] Revert "core(gather-runner): always reset scroll position (#8625)" This reverts commit 6bc4f89aa1896db07ea3b98172fb97423e2d0ce0. --- lighthouse-core/config/default-config.js | 1 + lighthouse-core/gather/driver.js | 41 +------------------ lighthouse-core/gather/gather-runner.js | 5 --- lighthouse-core/test/gather/driver-test.js | 13 ------ lighthouse-core/test/gather/fake-driver.js | 9 ---- .../test/gather/gather-runner-test.js | 28 ------------- 6 files changed, 2 insertions(+), 95 deletions(-) diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index fc5f152cbd30..33ac027e8e01 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -135,6 +135,7 @@ 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', ], }, diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 853cb328ea3b..93f267a8ae85 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -86,16 +86,6 @@ class Driver { this._handleReceivedMessageFromTarget(event, []).catch(this._handleEventError); }); - // We use isolated execution contexts for `evaluateAsync` that can be destroyed through navigation - // and other page actions. Cleanup our relevant bookkeeping as we see those events. - this.on('Runtime.executionContextDestroyed', event => { - if (event.executionContextId === this._isolatedExecutionContextId) { - this._clearIsolatedContextId(); - } - }); - - this.on('Page.frameNavigated', () => this._clearIsolatedContextId()); - connection.on('protocolevent', this._handleProtocolEvent.bind(this)); /** @@ -474,20 +464,7 @@ class Driver { */ async evaluateAsync(expression, options = {}) { const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined; - - try { - // `await` is not redunant here because we want to `catch` the async errors - return await this._evaluateInContext(expression, contextId); - } catch (err) { - // 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; - } + return this._evaluateInContext(expression, contextId); } /** @@ -1347,22 +1324,6 @@ class Driver { return flattenedDocument.nodes ? flattenedDocument.nodes : []; } - /** - * @param {{x: number, y: number}} position - * @return {Promise} - */ - 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} diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index bc719adc58db..46388f345321 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -324,10 +324,6 @@ class GatherRunner { 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 = pageLoadError ? null : await driver.getScrollPosition(); - for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; const status = { @@ -345,7 +341,6 @@ class GatherRunner { gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; await artifactPromise.catch(() => {}); - if (scrollPosition) await driver.scrollTo(scrollPosition); log.timeEnd(status); } log.timeEnd(apStatus); diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 8e3b0d069aee..d96ee191b1b9 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -317,19 +317,6 @@ 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: 'mocked value'}}); - - const value = await driver.evaluateAsync('"magic"', {useIsolation: true}); - expect(value).toEqual('mocked value'); - }); }); describe('.sendCommand', () => { diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 108a40aa6d78..1db517772bea 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -6,8 +6,6 @@ 'use strict'; function makeFakeDriver({protocolGetVersionResponse}) { - let scrollPosition = {x: 0, y: 0}; - return { getBrowserVersion() { return Promise.resolve(Object.assign({}, protocolGetVersionResponse, {milestone: 71})); @@ -59,13 +57,6 @@ function makeFakeDriver({protocolGetVersionResponse}) { evaluateAsync() { return Promise.resolve({}); }, - scrollTo(position) { - scrollPosition = position; - return Promise.resolve(); - }, - getScrollPosition() { - return Promise.resolve(scrollPosition); - }, registerPerformanceObserver() { return Promise.resolve(); }, diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 2a3a0755c7a9..6dca2bffd0c3 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -588,34 +588,6 @@ 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();