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

Extract information on cancelability of events #1534

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Conversation

dontcallmedom
Copy link
Member

This extracts the cancelability of all actually-cancelable events listed in #772 (comment), except for the copy/cut/paste events which would need a patch (the same way they need one for bubbling), and for dblclick (due to a bug in the spec)

I've also verified that all the events that get extracted as cancelable are indeed marked as such in the source spec.

I don't know of a good way to check that it's not missing other cancelable events.

This is in the context of https://github.com/orgs/mdn/discussions/665#discussioncomment-9008930

This doesn't have test, nor does it update the aggregation/validation code, hence marking it as draft (happy for someone else to take a stab at those :)

@dontcallmedom dontcallmedom marked this pull request as ready for review April 5, 2024 07:46
@dontcallmedom
Copy link
Member Author

now with test and updated schema

@tidoust
Copy link
Member

tidoust commented Apr 5, 2024

This part looks good to me.

I suppose the aggregation code still needs to be updated to add a setCancelablePerTarget method (or to amend setBubblingPerTarget), right? Or is the expectation that the cancelable will not depend on the target interface? If the latter, we'd need a test to make sure that the assumption is correct.

@dontcallmedom
Copy link
Member Author

thanks for the reminder re aggregation :)

AFAICT cancelability is orthogonal to the target interface; in some cases, cancelability is not systematic, which this extraction doesn't capture, but it's not obvious to me that we can or should try to do so; I wouldn't try until we have a clear use case.

The `cancelable` property is copied as-is by the post-processor, which should
be what we want.
In theory, specs could set or unset the `cancelable` property of a given event
differently whenever they fire the event. In practice, the cancelability of an
event does not depend on the firing conditions. In other words, whenever a spec
fires a given event, its `cancelable` property should always be the same.

That assumption seems true today. To ease debugging in case that changes in the
future, this update makes the extraction code report an error to the log when it
bumps into discrepancies for the `cancelable` property.

(Of course, ideally, we would have a better mechanism to report such extraction
problems than hiding them in the log...)
@tidoust
Copy link
Member

tidoust commented Apr 19, 2024

thanks for the reminder re aggregation :)

In practice, if we consider that cancelability is an intrinsic property of an event, the aggregation logic does not need to change. It already copies the cancelable property over to the aggregated file. I just updated the related JSON schema to add the property.

I also adjusted the extraction logic to report an error to the log if it finds the same event twice in a spec with different cancelable properties. I tested locally, that never happens.

Now, one last thing I'm wondering about is the nuance between undefined and false (that also applies to bubbles with the additional nuance that bubbles is meaningless when an event is not fired in a bubbling tree).

When I run this code against the Presentation API, it tells me that the spec defines a connectionavailable event that is not cancelable because the spec explicitly says that. But it tells me nothing about the cancelability of the change event, because the spec is not explicit about it.

But then when a spec uses a "fire an event" phrasing without specifying anything, the created event will not be cancelable (unless a spec one day decides to change cancelable in the init dictionary that extends EventInit but no spec does that AFAICT). In theory, the Presentation API could drop "The event must not bubble and must not be cancelable" when it fires connectionavailable and that would not change anything. Should we set cancelable to false (and possibly bubbles too) by default when we find a "fire an event" phrasing?

Maybe we don't really care as the most interesting case is probably to know when an event is cancelable.

Running the crawler locally with this update, I see 74 cancelable properties in the result (41 times set to false, 33 times set to true) out of the 441 events that we extract (not applying the events curation that we have in Webref). We have 127 events associated with a "fire an event phrasing".

@dontcallmedom
Copy link
Member Author

The false vs undefined question also arose in the MDN discussion that started this (in the context of bubbles); I would suggest we merge this as, where false reflects the spec being explicit about it, although I agree that in practice we may just need to track cancelable: true in the end. I think we'll be in a better position to make that change once we get consumers.

@tidoust tidoust merged commit 650506d into main Apr 19, 2024
1 check passed
@tidoust tidoust deleted the event-cancelability branch April 19, 2024 14:04
tidoust added a commit that referenced this pull request Apr 19, 2024
New feature:
- Extract information on cancelability of events (#1534)

Note the events extracts (and the consolidated `events.json` file)
gain a new `cancelable` property with this change.

Dependencies bumped:
- Bump puppeteer from 22.6.3 to 22.6.5 (#1543)
- Bump rollup from 4.14.1 to 4.14.3 (#1544)
- Bump ajv-formats from 2.1.1 to 3.0.1 (#1529)
- Bump undici from 6.12.0 to 6.13.0 (#1539)
- Bump web-specs from 3.7.1 to 3.8.0 (#1545)
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

Successfully merging this pull request may close these issues.

2 participants