DBW: Add audit for passive event listeners #830
Conversation
|
@RByers for the FYI |
| return PassiveEventsAudit.generateAuditResult({ | ||
| rawValue: results.length === 0, | ||
| extendedInfo: { | ||
| formatter: Formatter.SUPPORTED_FORMATS.URLLIST, |
There was a problem hiding this comment.
Do you want to handle the text wrapping?
| const Audit = require('../audit'); | ||
| const Formatter = require('../../formatters/formatter'); | ||
|
|
||
| const SCROLL_BLOCKING_EVENTS = [ |
There was a problem hiding this comment.
Can you add a comment pointing to something in the Blink source that confirms this list?
I think this is probably the best link: https://cs.chromium.org/search/?q=f:webinputevent.h+blocking&sq=package:chromium&type=cs
There was a problem hiding this comment.
| // not call preventDefault() are scroll blocking events. | ||
| const results = listeners.filter(loc => { | ||
| const isScrollBlocking = SCROLL_BLOCKING_EVENTS.indexOf(loc.type) !== -1; | ||
| const callsPreventDefault = loc.handler.description.match( |
There was a problem hiding this comment.
maybe call this mentionsPreventDefault?
| class PageLevelEventListeners extends Gatherer { | ||
|
|
||
| listenForScriptParsedEvents() { | ||
| return this.driver.sendCommand('Debugger.enable').then(_ => { |
There was a problem hiding this comment.
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:
| const matchedListeners = []; | ||
|
|
||
| return this._listEventListeners(location).then(results => { | ||
| const parsedScriptIds = this._parsedScripts.map(script => script.scriptId); |
There was a problem hiding this comment.
wanna store _parsedScripts as a map object instead (by scriptId), so you dont have to do this .map() each time?
There was a problem hiding this comment.
I need all the goodies in each parsed script later when creating combo
There was a problem hiding this comment.
clarified offline what I meant. Not a huge deal, though.
There was a problem hiding this comment.
Since multiple parsed scripts can share the same scriptId, I'm not sure the Map approach would work. I'd be able to look things up by scriptId, but it wouldn't (necessarily) be the correct value. I might be dumb.
..wait....
| const Formatter = require('../../formatters/formatter'); | ||
|
|
||
| const SCROLL_BLOCKING_EVENTS = [ | ||
| 'wheel', 'mousewheel', 'touchstart', 'touchmove' |
There was a problem hiding this comment.
wanna move these to a getBlockingEvents method on the audit so you dont have to duplicate in the test?
|
|
||
| // Note: line/col numbers are zero-index. Add one to each so we have | ||
| // actual file line/col numbers. | ||
| combo.line = combo.lineNumber + 1; |
There was a problem hiding this comment.
do you want to check that handler, url exist before assuming the line/col are correct and exist?
There was a problem hiding this comment.
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?
|
PTAL |
There was a problem hiding this comment.
feels good.
Can't think of other audits that could reuse this gatherer, but it's the right shape for it.
The other area of concern is flipping on Debugger domain in beforePass. I don't think this raises any particular concerns right now, but it's something for us to keep in mind.
lgtm
|
We'll rock the gatherer in #786 |
R: @paulirish @brendankenny @GoogleChrome/lighthouse
This audit adds:
window,document,document.body)touchstart,touchmove,wheel) that are not{passive: true}and which do not callpreventDefault().The gatherer results:
Report:
