-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Single event entries for TextTrack APIs #13887
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some checking of the entries where the versions don't match the parent feature. All looks good!
Resolved the merge conflict (caused by #13936) and added Webview Android versions from that PR to the eventName_event entries. |
"version_added": "10" | ||
}, | ||
"opera": { | ||
"version_added": "≤12.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second review I see that the "oncuechange" and "cuechange_event" data doesn't match for Opera or Safari. There's probably a lot of small differences. Can you list what they are for ease of review?
Such differences are really the only thing worth looking at when reviewing these PRs, so maybe it's worth writing a script to list the differences and generate a boilerplate comment for each PR?
Here are the differences: api.Texttrack.cuechange_event
api.Texttrack.oncuechange
api.TextTrackList.removetrack_event
api.TextTrackList.onremovetrack
|
My assumption is that the onremovetrack and the oncuechange entries are more correct because these were updated by the collector. |
Yes, I'd take the For "cuechange_event", I think this would be a good guess that's fairly consistent with the other data:
|
Another thing here is api.GlobalEventHandlers.oncuechange. browser-compat-data/api/GlobalEventHandlers.json Line 1016 in 44310a2
My guess would be to remove this data as it is captured in api.TextTrack.cuechange_event and api.HTMLTrackElement.cuechange_event. Not sure where to redirect https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/oncuechange to. Either to https://developer.mozilla.org/en-US/docs/Web/API/TextTrack/cuechange_event or to https://developer.mozilla.org/en-US/docs/Web/API/HTMLTrackElement/cuechange_event |
42fd335
to
81c18ec
Compare
81c18ec
to
24b1743
Compare
I redirected to TextTrack. Appreciate a look at the content change, too: mdn/content#12684 Will remove api.GlobalEventHandlers.oncuechange here then, too. |
I made the mistake of checking if the two remaining "cuechange_event" entries on A trivial issue is that a link to https://html.spec.whatwg.org/multipage/webappapis.html#handler-oncuechange is missing. https://trac.webkit.org/changeset/100064/webkit introduced the two events in WebKit, so the versions for Safari should match, but they don't. The data for I doubt the data for Opera is correct, and would suggest setting it to "12" like most of There are also differences in the IE and Edge data that I haven't researched, but I wouldn't be surprised if it's wrong. Also, should we capture that the |
Thanks for your detailed view. Unfortunately I don't know all the answers.
Done.
Done.
I've added a range to show partial support. The wording is per our guideline: https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines.md#dom-events-eventname_event For the issues with IE support and Safari versions, I don't know how to resolve the issues you found. You could file it as a new issue and we land this to continue refactoring other event data? |
{ | ||
"version_added": "6", | ||
"partial_implementation": true, | ||
"notes": "The <code>oncuechange</code> event handler property is not supported." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a note like this for Chrome and friends too. Using http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=10032 I found that it was added in Chrome 32.
If this is approved, it can be merged. The content work got merged in mdn/content#12688 and mdn/content#12684 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a mix of Safari/iOS versions that must be wrong, with 6/6 in some places and 6/7 in others. But that problem exists outside of the updated data, so let's leave that for another day.
Per the new guideline: https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines.md#dom-events-eventname_event
Content work to be done.