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

Add HTML Media types #802

Merged
merged 5 commits into from
Mar 17, 2020
Merged

Add HTML Media types #802

merged 5 commits into from
Mar 17, 2020

Conversation

saschanaz
Copy link
Contributor

No description provided.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This deletes a lot of MS-specific properties. Have we done that in the past? Which browsers will be broken? If a lot of people are still targetting those browsers then we should maybe keep the MS-specific stuff around.

@saschanaz
Copy link
Contributor Author

Have we done that in the past?

Yes, several MS- and WebKit-specific types have been removed since we started to use Web IDL. Some of them are listed in removedTypes.json but generally are just overridden without manual list.

Which browsers will be broken? If a lot of people are still targetting those browsers then we should maybe keep the MS-specific stuff around.

AFAICT the removed types here are:

  1. Old IE-era APIs that have been removed from Edge
  2. Some MS-specific DRM APIs which are kept in Edge <= 18 (Not available on Chromium-based Edge)

I think use of DRM should be rare enough.

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Had a look through this PR, I get that it removes a bunch of the old IE-y bits in favor of an IDL which is a big 👍 but it's replacing it with the API which is still flagged

The Media Playback Quality change we can insta-merge, but until the "HTML - Media" is un-flagged in either FF/Chromium we shouldn't be adding it to the main libs

{
"url": "https://html.spec.whatwg.org/multipage/media.html",
"title": "HTML - Media"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the controversial one: It's only supported in Safari and old edge today (without flags ) according to caniuse:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know that, nice find! But those types are already in lib.d.ts so removing them might affect existing users. Should I remove them anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

We think of it as the status quo is wrong, yep, but adding the new spec would make people expect something exists in the runtime which doesn't (without turning on a flag for 90% of the web).

If it gets removed from the PR is it still feasible for the media playback to come in? Then we keep the dodgy existing behavior until the runtime features available for everyone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec itself also covers an essential part of HTML - HTMLAudio/Video/TrackElement, so I think the spec should still be added, while we can drop specific types mentioned here.

If it gets removed from the PR is it still feasible for the media playback to come in?

It shouldn't be affected, as it only adds a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I agree - let's do that 👍

{
"url": "https://w3c.github.io/media-playback-quality/",
"title": "Media Playback Quality"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This one seems totally fine, it's an editor's draft - yep, but every browser seems to be supporting it

@saschanaz
Copy link
Contributor Author

Had to also remove SourceBuffer.prototype.textTracks as it turns out both Firefox and Chrome does not have it. I filed this on BCD.

@@ -241,6 +249,9 @@
},
"interfaces": {
"interface": {
"AudioTrack": {
"exposed": ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a way to properly exclude certain types from unions.

@sandersn sandersn merged commit 099e238 into microsoft:master Mar 17, 2020
@saschanaz saschanaz deleted the media branch March 17, 2020 16:33
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Mar 20, 2020
AudioTrackList isn't standard, and is removed from the DOM in TS 3.9:
  microsoft/TypeScript-DOM-lib-generator#802

This PR removes it from dom-mediacapture-record
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Mar 23, 2020
AudioTrackList isn't standard, and is removed from the DOM in TS 3.9:
  microsoft/TypeScript-DOM-lib-generator#802

This PR removes it from dom-mediacapture-record
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Mar 24, 2020
AudioTrack isn't standard (only supported in IE and Safari), and is
removed from the DOM lib in TS 3.9: microsoft/TypeScript-DOM-lib-generator#802

This PR puts a copy of the type called VideojsAudioTrack in instead.
That should be compatible with the DOM version for older Typescripts,
since it's copied directly from the old source.
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Mar 24, 2020
AudioTrack isn't standard (only supported in IE and Safari), and is
removed from the DOM lib in TS 3.9: microsoft/TypeScript-DOM-lib-generator#802

This PR puts a copy of the type called VideojsAudioTrack in instead.
That should be compatible with the DOM version for older Typescripts,
since it's copied directly from the old source.
}

/** Used to represent a list of the audio tracks contained within a given HTML media element, with each track represented by a separate AudioTrack object in the list. */
interface AudioTrackList extends EventTarget {
Copy link
Member

Choose a reason for hiding this comment

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

Why were these removed? They're part of the DOM standard: https://html.spec.whatwg.org/multipage/media.html#audiotracklist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but unfortunately nobody has a stable implementation per MDN and TSJS-lib-generator generally should only include stable ones.

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.

4 participants