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

Guarantee that there are events associated with interfaces that inherit from EventTarget #1216

Open
tidoust opened this issue Apr 16, 2024 · 1 comment

Comments

@tidoust
Copy link
Member

tidoust commented Apr 16, 2024

Via #1212 (comment).

We already use the fact that an interface inherits from EventTarget to find target interfaces for events when we extract them from the specs, but we don't have test in place that checks that:

  1. All interfaces that inherit from EventTarget are targeted by at least one event (for example: there should be events that target CloseWatcher)
  2. We have a suitably named event for all EventHandler attributes in such interfaces (for example: there should a cancel event that targets CloseWatcher because it has an oncancel attribute.

Depending on what research reveals, this test could be turned into a guarantee for the @webref/events package. There may be exceptions to the rule though (such as new interfaces for which the draft spec does not yet correctly define associated events)

@tidoust
Copy link
Member Author

tidoust commented Apr 22, 2024

I've been looking into adding a check to Strudy's study-webidl. A couple of things make that slightly painful:

  • We want the more specific target when possible when the event handler is defined on a common ancestor interface. Logic needs to follow inheritance chains as a result. That's ok, it's relatively easy to check that in practice.
  • An interface may end up with an EventHandler attribute even though the related event does not fire at it, when the interfaces are part of a bubbling tree, and when the event bubbles. That's more painful to test: either we start from the consolidated events.json file or we need to expose Reffy's bubbling logic somehow. In practice, we're only talking about a couple of events in IndexedDB, most other bubbling events get simply captured through inheritance (in HTML, event handlers are defined on HTMLElement).

I'll try to prepare something relatively clean for integration in Strudy. In the meantime, the analysis reveals that we could almost guarantee that "all event handlers get targeted by a suitable event", but for the following cases:

  • No orientationchange event found for HTMLBodyElement.onorientationchange (compat).
    This one is interesting because we're the ones who remove the HTMLBodyElement target. The IDL definition of HTMLBodyElement does have the event handler though. I guess that's going to remain an exception to the rule? (That is, unless you're on iOS Safari, the event handler will indeed never be called).
  • No error event found for RTCIceTransport.onerror (webrtc-ice).
    Also interesting because there is an event but its name is icecandidateerror. That seems to be the only case where the name of the event handler attribute is not the expected one. Any reason why the event handler was not named onicecandidateerror?

[Edit: problems reported and fixed] This also reveals 4 other problems in WebXR Layers, where the redraw event is defined to target the root XRLayer interface, whereas the event handlers are defined on specific interfaces that inherit from XRLayer. I'll report the errors:

  • No redraw event found for XRQuadLayer.onredraw (webxrlayers-1)
  • No redraw event found for XRCylinderLayer.onredraw (webxrlayers-1)
  • No redraw event found for XREquirectLayer.onredraw (webxrlayers-1)
  • No redraw event found for XRCubeLayer.onredraw (webxrlayers-1)

While digging into data, I also bumped into the definition of XHR events, which are currently defined with a data-dfn-for="XMLHttpRequest" attribute, while they also fire on XMLHttpRequestUpload. The attribute should rather be data-dfn-for="XMLHttpRequestEventTarget".

tidoust added a commit to tidoust/layers that referenced this issue Apr 22, 2024
The `redraw` event was defined with a `for="XRLayer"` attribute. That is not
really correct, because the even can only fire at some of the
`XRCompositionLayer` interfaces. This update adjusts the `for` attribute
accordingly, making the definition consistent with the IDL definitions of the
related `EventHandler` attributes.

(Via w3c/webref#1216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant