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

Demix GlobalEventHandlers; adapt to new events structure #15480

Closed
wants to merge 8 commits into from

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Mar 23, 2022

This PR performs two operations relating to the GlobalEventHandlers mixin.

First, the mixin is de-mixed and separated into their individual interfaces. This includes Document, Window, HTMLElement,
MathMLElement, and SVGElement.

Second, the event handlers are converted to the new naming convention, and URLs are also updated. In the process, duplicates are removed.

Possible conflict with #15677.

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Mar 23, 2022
@queengooborg queengooborg added the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Mar 23, 2022
Copy link
Collaborator

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that for the most part these should be a single event entry, and it needs to be decided on a case-by-case basis. Some should be multiple entries still, a few events like "blur" can be fired at both Window and some elements and have different meanings for those.

There are far too many events to do this in batch and review the result properly, I suggest splitting into many small PRs.

}
}
},
"canplay_event": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entry isn't meaningful, no event will ever be fired for document.addEventListener('canplay', handler) because the event target is an HTMLMediaElement and the events doesn't bubble.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we have:
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/oncanplay
https://developer.mozilla.org/en-US/docs/web/api/htmlmediaelement/canplay_event

Per IDL, it would be possible to have the event on:
Window
Document
HTMLElement
MathMLElement
SVGElement

I wonder if we wouldn't be better off if we have the events under GlobalEventHandlers in BCD. On MDN we also place them under GlobalEventHandlers and document where they are available (list above) and where they are typically used (in this case HTMLMediaElements like video and audio). What do you think?

@queengooborg queengooborg added not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. and removed not ready ⛔ This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. labels Apr 7, 2022
@queengooborg queengooborg marked this pull request as draft April 7, 2022 17:23
@queengooborg
Copy link
Collaborator Author

I'm marking this as a draft because there is some inaccurate flag data in this mixin that I'd like to get removed first before this is merged.

@Elchi3
Copy link
Member

Elchi3 commented Apr 22, 2022

There are far too many events to do this in batch and review the result properly, I suggest splitting into many small PRs.

Agree. I think we should close this PR and work our way through GlobalEventHandlers piece by piece. However, I would suggest to open one or two PRs for the moment that deal with one or two events only. Then we can play through what updates we want to make exactly in BCD and also on MDN for these events. Once we are clear, we can open a lot more PRs given we then know what we're doing.

@queengooborg
Copy link
Collaborator Author

I'm going to close this PR and create other PRs for each individual event instead, so that each one can be reviewed individually.

@queengooborg queengooborg deleted the api/GlobalEventHandlers branch June 2, 2022 05:21
@queengooborg queengooborg removed the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants