diff --git a/src/lib/study-webidl.js b/src/lib/study-webidl.js index eac282cf..848fdd3d 100644 --- a/src/lib/study-webidl.js +++ b/src/lib/study-webidl.js @@ -20,6 +20,7 @@ */ import * as WebIDL2 from 'webidl2'; +import { getInterfaceTreeInfo } from 'reffy'; const getSpecs = list => [...new Set(list.map(({ spec }) => spec))]; const specName = spec => spec.shortname ?? spec.url; @@ -174,6 +175,15 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) const usedTypes = {}; // Index of types used in the IDL const usedExtAttrs = {}; // Index of extended attributes + // We need to run the analysis on all specs, even if caller is only + // interested in a few of them, because types may be defined in specs that + // the caller is not interested in. + const allSpecs = (crawledResults.length > 0) ? crawledResults : specs; + const allEvents = allSpecs + .filter(spec => Array.isArray(spec.events)) + .map(spec => spec.events.map(e => Object.assign({ spec }, e))) + .flat(); + // Record an anomaly for the given spec(s), // provided we are indeed interested in the results function recordAnomaly (spec, name, message) { @@ -193,17 +203,54 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) function inheritsFrom (iface, ancestor) { if (!iface.inheritance) return false; if (iface.inheritance === ancestor) return true; + if (!dfns[iface.inheritance]) return false; const parentInterface = dfns[iface.inheritance].find(({ idl }) => !idl.partial)?.idl; if (!parentInterface) return false; return inheritsFrom(parentInterface, ancestor); } + function getBubblingPath(iface) { + const interfaces = Object.values(dfns) + .map(dfn => dfn.find(d => d.idl.type === 'interface')) + .filter(dfn => !!dfn) + .map(dfn => dfn.idl); + const { bubblingPath } = getInterfaceTreeInfo(iface, interfaces); + return bubblingPath; + } + // TODO: a full check of event handlers would require: // - checking if the interface doesn't include a mixin with eventhandlers function checkEventHandlers (spec, memberHolder, iface = memberHolder) { - const eventHandler = memberHolder.members.find(m => m?.name?.startsWith('on') && m.type === 'attribute' && m.idlType?.idlType === 'EventHandler'); - if (eventHandler && !inheritsFrom(iface, 'EventTarget')) { - recordAnomaly(spec, 'unexpectedEventHandler', `The interface \`${iface.name}\` defines an event handler \`${eventHandler.name}\` but does not inherit from \`EventTarget\``); + const eventHandlers = memberHolder.members.filter(m => m?.name?.startsWith('on') && m.type === 'attribute' && m.idlType?.idlType === 'EventHandler'); + if (eventHandlers.length > 0 && !inheritsFrom(iface, 'EventTarget')) { + recordAnomaly(spec, 'unexpectedEventHandler', `The interface \`${iface.name}\` defines an event handler \`${eventHandlers[0].name}\` but does not inherit from \`EventTarget\``); + } + + for (const eventHandler of eventHandlers) { + const eventType = eventHandler.name.substring(2); + const events = allEvents.filter(e => e.type === eventType); + let event = events.find(e => e.targets?.find(target => + target === iface.name)); + if (!event) { + // No event found with the same type that targets the interface + // directly, but one the events may target an interface that + // inherits from the interface, or may bubble on the interface. + event = events.find(e => e.targets?.find(target => { + const inheritanceFound = dfns[target]?.find(dfn => + dfn.idl.type === 'interface' && + inheritsFrom(dfn.idl, iface.name)); + if (inheritanceFound) return true; + if (!e.bubbles) return false; + const bubblingPath = getBubblingPath(target); + if (!bubblingPath) return false; + return bubblingPath.find(bubblingIface => + iface.name === bubblingIface || + inheritsFrom(iface, bubblingIface)); + })); + } + if (!event) { + recordAnomaly(spec, 'noEvent', `The interface \`${iface.name}\` defines an event handler \`${eventHandler.name}\` but no event named "${eventType}" targets it`); + } } } @@ -384,10 +431,6 @@ function studyWebIdl (specs, { crawledResults = [], curatedResults = [] } = {}) } } - // We need to run the analysis on all specs, even if caller is only - // interested in a few of them, because types may be defined in specs that - // the caller is not interested in. - const allSpecs = (crawledResults.length > 0) ? crawledResults : specs; allSpecs // We're only interested in specs that define Web IDL content .filter(spec => !!spec.idl) diff --git a/src/lib/study.js b/src/lib/study.js index ce52bf09..3ff86098 100644 --- a/src/lib/study.js +++ b/src/lib/study.js @@ -149,6 +149,11 @@ const anomalyGroups = [ guidance: 'See the [`[Exposed]`](https://webidl.spec.whatwg.org/#Exposed) extended attribute section in Web IDL for requirements.' }, { name: 'invalid', title: 'Invalid Web IDL' }, + { + name: 'noEvent', + title: 'Event handlers with no matching events', + description: 'No matching events were found for the following event handlers' + }, { name: 'noExposure', title: 'Missing `[Exposed]` attributes' }, { name: 'noOriginalDefinition', title: 'Missing base interfaces' }, { name: 'overloaded', title: 'Invalid overloaded operations' }, diff --git a/test/study-webidl.js b/test/study-webidl.js index 0d3b3e7c..b970bcb6 100644 --- a/test/study-webidl.js +++ b/test/study-webidl.js @@ -7,6 +7,16 @@ import study from '../src/lib/study-webidl.js'; import { assertNbAnomalies, assertAnomaly } from './util.js'; +const baseEventIdl = ` +[Exposed=*] +interface Event {}; +[LegacyTreatNonObjectAsNull] +callback EventHandlerNonNull = any (Event event); +typedef EventHandlerNonNull? EventHandler; +[Exposed=*] +interface EventTarget {}; +`; + describe('The Web IDL analyser', () => { const specUrl = 'https://www.w3.org/TR/spec'; const specUrl2 = 'https://www.w3.org/TR/spec2'; @@ -148,42 +158,90 @@ interface WhereIAm {}; }); }); - it('reports no anomaly for valid EventHandler attributes definitions', () => { - const report = analyzeIdl(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + it('reports EventHandler attributes with no matching events', () => { + const report = analyzeIdl(baseEventIdl + ` [Global=Window,Exposed=*] interface Carlos : EventTarget { attribute EventHandler onbigbisous; }; -[Exposed=*] -interface EventTarget {}; `); + assertNbAnomalies(report, 1); + assertAnomaly(report, 0, { + name: 'noEvent', + message: 'The interface `Carlos` defines an event handler `onbigbisous` but no event named "bigbisous" targets it' + }); + }); + + it('reports no anomaly for valid EventHandler attributes definitions', () => { + const crawlResult = toCrawlResult(baseEventIdl +` +[Global=Window,Exposed=*] +interface Carlos : EventTarget { + attribute EventHandler onbigbisous; +};`); + crawlResult[0].events = [{ + type: 'bigbisous', + interface: 'Event', + targets: ['Carlos'] + }]; + const report = study(crawlResult); assertNbAnomalies(report, 0); }); it('detects unexpected EventHandler attributes', () => { - const report = analyzeIdl(` -[Exposed=*] -interface Event {}; -[LegacyTreatNonObjectAsNull] -callback EventHandlerNonNull = any (Event event); -typedef EventHandlerNonNull? EventHandler; - + const report = analyzeIdl(baseEventIdl + ` [Global=Window,Exposed=*] interface Carlos { attribute EventHandler onbigbisous; -}; -`); - assertNbAnomalies(report, 1); +};`); + assertNbAnomalies(report, 2); assertAnomaly(report, 0, { name: 'unexpectedEventHandler', message: 'The interface `Carlos` defines an event handler `onbigbisous` but does not inherit from `EventTarget`' }); + assertAnomaly(report, 1, { name: 'noEvent' }); + }); + + it('follows the inheritance chain to assess event targets', () => { + const crawlResult = toCrawlResult(baseEventIdl + ` +[Global=Window,Exposed=*] +interface Singer : EventTarget { + attribute EventHandler onbigbisous; +}; + +[Global=Window,Exposed=*] +interface Carlos : Singer {};`); + crawlResult[0].events = [{ + type: 'bigbisous', + interface: 'Event', + targets: [ + 'Carlos' + ] + }]; + const report = study(crawlResult); + assertNbAnomalies(report, 0); + }); + + it('follows the bubbling path to assess event targets', () => { + const crawlResult = toCrawlResult(baseEventIdl + ` +[Global=Window,Exposed=*] +interface IDBDatabase : EventTarget { + attribute EventHandler onabort; +}; +[Global=Window,Exposed=*] +interface IDBTransaction : EventTarget { + attribute EventHandler onabort; +}; +[Global=Window,Exposed=*] +interface MyIndexedDB : IDBDatabase { +};`); + crawlResult[0].events = [{ + type: 'abort', + interface: 'Event', + targets: ['IDBTransaction'], + bubbles: true + }]; + const report = study(crawlResult); + assertNbAnomalies(report, 0); }); it('detects incompatible partial exposure issues', () => {