Skip to content

Commit

Permalink
Web IDL study: detect event handlers with no matching event (#785)
Browse files Browse the repository at this point in the history 
When an interface defines an event handler, there should always be an event
named after the event handler that targets the interface. The new `noEvent`
anomaly detects situations where the event is missing.

The analysis reports the two anomalies mentioned in:
w3c/webref#1216 (comment)

(The analysis reports additional anomalies when run on the raw extracts. That's
normal and due to missing event targets that get added during curation)
  • Loading branch information
@tidoust
tidoust authored Sep 11, 2024
1 parent 75ffbc3 commit fb6cec0
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 27 deletions.
57 changes: 50 additions & 7 deletions src/lib/study-webidl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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`);
}
}
}

Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions src/lib/study.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand Down
98 changes: 78 additions & 20 deletions test/study-webidl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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', () => {
Expand Down

0 comments on commit fb6cec0

Please sign in to comment.