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

DBW: Add audit for passive event listeners #830

Merged
merged 6 commits into from
Oct 29, 2016
Merged
Show file tree
Hide file tree
Changes from all 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
95 changes: 95 additions & 0 deletions lighthouse-core/audits/dobetterweb/uses-passive-event-listeners.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @fileoverview Audit a page to see if it is using passive event listeners on
* document-level event listeners (e.g. on window, document, document.body).
*/

'use strict';

const url = require('url');
const Audit = require('../audit');
const Formatter = require('../../formatters/formatter');

class PassiveEventsAudit extends Audit {

static get SCROLL_BLOCKING_EVENTS() {
// See https://github.com/WICG/EventListenerOptions/blob/gh-pages/explainer.md
return ['wheel', 'mousewheel', 'touchstart', 'touchmove'];
}

/**
* @return {!AuditMeta}
*/
static get meta() {
return {
category: 'JavaScript',
name: 'uses-passive-event-listeners',
description: 'Site is using passive listeners (at the page level) ' +
'to improve scrolling performance',
helpText: `<a href="https://www.chromestatus.com/features/5745543795965952" target="_blank">Passive event listeners</a> enable better scrolling performance. If you don't call <code>preventDefault()</code> in your <code>${this.SCROLL_BLOCKING_EVENTS.toString()}</code> event listeners, make them passive: <code>addEventListener('touchstart', ..., {passive: true})</code>.`,
requiredArtifacts: ['URL', 'PageLevelEventListeners']
};
}

/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static audit(artifacts) {
if (typeof artifacts.PageLevelEventListeners === 'undefined' ||
artifacts.PageLevelEventListeners === -1) {
return PassiveEventsAudit.generateAuditResult({
rawValue: -1,
debugString: 'PageLevelEventListeners gatherer did not run'
});
} else if (artifacts.PageLevelEventListeners.rawValue === -1) {
return PassiveEventsAudit.generateAuditResult(artifacts.PageLevelEventListeners);
}

const listeners = artifacts.PageLevelEventListeners;
const pageHost = url.parse(artifacts.URL.finalUrl).host;

// Filter out non-passive window/document/document.body listeners that do
// not call preventDefault() are scroll blocking events.
const results = listeners.filter(loc => {
const isScrollBlocking = this.SCROLL_BLOCKING_EVENTS.indexOf(loc.type) !== -1;
const mentionsPreventDefault = loc.handler.description.match(
/\.preventDefault\(\s*\)/g);
const sameHost = url.parse(loc.url).host === pageHost;
return sameHost && isScrollBlocking && !loc.passive &&
!mentionsPreventDefault;
}).map(loc => {
const handler = loc.handler ? loc.handler.description : '...';
return Object.assign({
label: `line: ${loc.line}, col: ${loc.col}`,
code: `${loc.objectId}.addEventListener('${loc.type}', ${handler})`
}, loc);
});

return PassiveEventsAudit.generateAuditResult({
rawValue: results.length === 0,
extendedInfo: {
formatter: Formatter.SUPPORTED_FORMATS.URLLIST,
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to handle the text wrapping?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

value: results
}
});
}
}

module.exports = PassiveEventsAudit;
5 changes: 5 additions & 0 deletions lighthouse-core/config/dobetterweb.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"../gather/gatherers/dobetterweb/appcache",
"../gather/gatherers/dobetterweb/console-time-usage",
"../gather/gatherers/dobetterweb/datenow",
"../gather/gatherers/dobetterweb/document-event-listeners",
"../gather/gatherers/dobetterweb/document-write",
"../gather/gatherers/dobetterweb/websql"
]
Expand All @@ -21,6 +22,7 @@
"../audits/dobetterweb/no-old-flexbox",
"../audits/dobetterweb/no-websql",
"../audits/dobetterweb/uses-http2",
"../audits/dobetterweb/uses-passive-event-listeners",
"../audits/is-on-https"
],

Expand Down Expand Up @@ -65,6 +67,9 @@
},
"no-console-time": {
"expectedValue": false
},
"uses-passive-event-listeners": {
"expectedValue": true
}
}
}, {
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/formatters/partials/url-list.html
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
.http-resource__code {
color: #999;
}
.http-resource__code {
text-overflow: ellipsis;
overflow: hidden;
}
</style>

<div>
Expand Down
5 changes: 4 additions & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,10 @@ class Driver {
options: 'sampling-frequency=10000' // 1000 is default and too slow.
};

return this.sendCommand('Page.enable')
return this.sendCommand('Debugger.disable')
.then(_ => this.sendCommand('DOM.disable'))
.then(_ => this.sendCommand('CSS.disable'))
.then(_ => this.sendCommand('Page.enable'))
.then(_ => this.sendCommand('Tracing.start', tracingOpts));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/**
* @license
* Copyright 2016 Google Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @fileoverview Tests whether the page is using passive event listeners.
*/

'use strict';

const Gatherer = require('../gatherer');

const LISTENER_LOCATIONS = ['window', 'document', 'document.body'];

class PageLevelEventListeners extends Gatherer {

listenForScriptParsedEvents() {
return this.driver.sendCommand('Debugger.enable').then(_ => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we're mostly good, but let's make sure this domain is off when we dont need it.

can you add Debugger.disable, DOM.disable, and CSS.disable to beginTrace()? That's what devtools does:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

this.driver.on('Debugger.scriptParsed', script => {
this._parsedScripts.set(script.scriptId, script);
});
});
}

unlistenForScriptParsedEvents() {
this.driver.off('Debugger.scriptParsed', this.listenForScriptParsedEvents);
return this.driver.sendCommand('Debugger.disable');
}

/**
* @param {string} expression An expression to evaluate in the page.
* @return {!Promise<!Array.<EventListener>>}
* @private
*/
_listEventListeners(expression) {
return this.driver.sendCommand('Runtime.evaluate', {
expression,
objectGroup: 'page-listeners-gatherer' // needed to populate .handler
}).then(result => {
return this.driver.sendCommand('DOMDebugger.getEventListeners', {
objectId: result.result.objectId
});
});
}

/**
* Collects the event listeners attached to an object and formats the results.
* listenForScriptParsedEvents should be called before this method to ensure
* the page's parsed scripts are collected at page load.
* @param {string} location An object to look for attached event listeners.
* @return {!Promise<!Array.<Object>>} List of event listeners attached to
* location.
*/
getEventListeners(location) {
const matchedListeners = [];

return this._listEventListeners(location).then(results => {
results.listeners.forEach(listener => {
// Slim down the list of parsed scripts to match the found event
// listeners that have the same script id.
const script = this._parsedScripts.get(listener.scriptId);
if (script) {
// Combine the EventListener object and the result of the
// Debugger.scriptParsed event so we get .url and other
// needed properties.
const combo = Object.assign(listener, script);
combo.objectId = location; // One of LISTENER_LOCATIONS.

// Note: line/col numbers are zero-index. Add one to each so we have
// actual file line/col numbers.
combo.line = combo.lineNumber + 1;
Copy link
Member

Choose a reason for hiding this comment

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

do you want to check that handler, url exist before assuming the line/col are correct and exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting line/col numbers even before setting objectGroup: 'page-listeners-gatherer'. If handler doesn't exist, it might be good to handle that instead in the audit extendedInfo.code. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗺 'd so hard

combo.col = combo.columnNumber + 1;

matchedListeners.push(combo);
}
});

return matchedListeners;
});
}

/**
* Aggregates the event listeners used on each object into a single list.
* @param {Array.<string>} locations Objects to look for attached event listeners.
* @return {!Promise<!Array.<Object>>} Resolves to a list of all the event
* listeners found on each object.
*/
collectListeners(locations) {
return locations.reduce((chain, location) => {
return chain.then(prevArr => {
return this.getEventListeners(location).then(results => {
return prevArr.concat(results);
});
});
}, Promise.resolve([]));
}

beforePass(options) {
this.driver = options.driver;
this._parsedScripts = new Map();
return this.listenForScriptParsedEvents();
}

afterPass(options) {
return this.unlistenForScriptParsedEvents(options.driver)
.then(_ => this.collectListeners(LISTENER_LOCATIONS))
.then(listeners => {
this.artifact = listeners;
}).catch(_ => {
this.artifact = {
usage: -1,
debugString: 'Unable to gather passive events listeners usage.'
};
});
}
}

module.exports = PageLevelEventListeners;
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* Copyright 2016 Google Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const PassiveEventsAudit = require('../../../audits/dobetterweb/uses-passive-event-listeners.js');
const assert = require('assert');
const fixtureData = require('../../fixtures/page-level-event-listeners.json');

const URL = 'https://example.com';

/* eslint-env mocha */

describe('Page uses passive events listeners where applicable', () => {
it('it returns error value when no input present', () => {
const auditResult = PassiveEventsAudit.audit({});
assert.equal(auditResult.rawValue, -1);
assert.ok(auditResult.debugString);
});

it('debugString is present if gatherer fails', () => {
const debugString = 'Unable to gather passive events listeners usage.';
const auditResult = PassiveEventsAudit.audit({
PageLevelEventListeners: {
rawValue: -1,
debugString: debugString
},
URL: {finalUrl: URL}
});
assert.equal(auditResult.rawValue, -1);
assert.equal(auditResult.debugString, debugString);
});

it('fails when page-level scroll blocking listeners should be passive', () => {
const auditResult = PassiveEventsAudit.audit({
PageLevelEventListeners: fixtureData,
URL: {finalUrl: URL}
});
assert.equal(auditResult.rawValue, false);

for (let i = 0; i < auditResult.extendedInfo.value.length; ++i) {
const val = auditResult.extendedInfo.value[i];
assert.ok(!val.passive, 'results should all be non-passive listeners');
assert.notEqual(PassiveEventsAudit.SCROLL_BLOCKING_EVENTS.indexOf(val.type), -1,
'results should not contain other types of events');
}
assert.equal(auditResult.extendedInfo.value.length, 5);
assert.equal(auditResult.extendedInfo.value[0].url, fixtureData[0].url);
assert.ok(auditResult.extendedInfo.value[0].code.match(/addEventListener/));
});

it('passes page-level scroll blocking listeners should be passive', () => {
const auditResult = PassiveEventsAudit.audit({
PageLevelEventListeners: [],
URL: {finalUrl: URL}
});
assert.equal(auditResult.rawValue, true);
assert.equal(auditResult.extendedInfo.value.length, 0);
});
});
Loading