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

#475@minor: Adds support for HTMLMediaElement. #535

Merged
merged 4 commits into from Oct 7, 2022

Conversation

rudywaltz
Copy link
Contributor

@rudywaltz rudywaltz commented Jul 9, 2022

Resolves #484
Resolves #475

It's a very basic implementation mostly covering the API (at least what is common between audio and video and widely supported in browsers) timeRanges implementation coming from jsdom as the comment mentioned.
I try to implement some basic logic but mostly is missing, I hope this can be good enough as a first step.

References:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement
https://html.spec.whatwg.org/#htmlmediaelement
https://github.com/jsdom/jsdom

p.s.: should I need to add HTMLAudioElement and HTMLVideoELement to window.ts and index.ts or the changes is the packages/happy-dom/src/config/ElementTag.ts in enough for use?

@Mas0nShi
Copy link
Contributor

although those tags extend from HTMLMediaElement, I think we should implement for HTMLVideoElement and HTMLAudioElement before removing or adding these tags in ElementTag.ts.

Of course this is my personal advice, perhaps repo owner's advice is more valuable.

@rudywaltz
Copy link
Contributor Author

Hi @Mas0nShi ,
thanks for the feedback.
HTMLAudioElement has a same API as HTMLMediaElement, only mozzilla-specific API is a difference if I understand correctly the documentation.
https://html.spec.whatwg.org/multipage/media.html#htmlaudioelement
(maybe better doing this here as well? creating an audio element and that is just extending the Media element? 🤔 )

HTMLVideoElement has some widely supported properties/attributes that are not coming from HTMLMediaElement, so yes maybe better implement that as well before adding or removing it from the not implemented list, but still after this, most of the API working in the test.
https://html.spec.whatwg.org/multipage/media.html#the-video-element

@Mas0nShi
Copy link
Contributor

(maybe better doing this here as well? creating an audio element and that is just extending the Media element? 🤔 )

yes, it's my advices, we can add HTMLMediaElement and HTMLVideoElement though identical or incomplete.

@rudywaltz
Copy link
Contributor Author

(maybe better doing this here as well? creating an audio element and that is just extending the Media element? 🤔 )

yes, it's my advices, we can add HTMLMediaElement and HTMLVideoElement though identical or incomplete.

done

@rudywaltz rudywaltz force-pushed the master branch 2 times, most recently from ddc215a to e211de5 Compare July 22, 2022 09:14
@rudywaltz rudywaltz changed the title 475@minor: Adds support for HTMLMediaElement. #475@minor: Adds support for HTMLMediaElement. Jul 22, 2022
@rudywaltz rudywaltz force-pushed the master branch 2 times, most recently from de18f41 to 9803545 Compare July 25, 2022 07:40
@capricorn86
Copy link
Owner

capricorn86 commented Oct 6, 2022

Hi @rudywaltz and @Mas0nShi ! 🙂

Sorry for not looking into this earlier. It has been a lot going on in my private and work life.

I have managed to get the PR build to work and it seems like this PR is failing to build.

I can merge this when the build is successful.

@capricorn86
Copy link
Owner

Hi @Mas0nShi ! 🙂

Sorry for not looking into this earlier. It has been a lot going on in my private and work life.

I have managed to get the PR build to work and it seems like this PR is failing to build.

I can merge this when the build is successful.

@rudywaltz
Copy link
Contributor Author

rudywaltz commented Oct 6, 2022

Hi @capricorn86 , thank you for taking care of this repo. Can I ask some hint where is the missing part. I see the error locally as well

 src/window/Window.ts(256,18): error TS2416: Property 'self' in type 'Window' is not assignable to the same property in base type 'IWindow'.
  Type 'this' is not assignable to type 'IWindow'.
    Property 'HTMLAudioElement' is missing in type 'Window' but required in type 'IWindow'.

but I have no idea where is the missing part. I try to follow the DialogElement implementation and I don't find any place where the Audio is missing ):

@capricorn86
Copy link
Owner

capricorn86 commented Oct 6, 2022

Hi @capricorn86 , thank you for taking care of this repo. Can I ask some hint where is the missing part. I see the error locally as well

 src/window/Window.ts(256,18): error TS2416: Property 'self' in type 'Window' is not assignable to the same property in base type 'IWindow'.
  Type 'this' is not assignable to type 'IWindow'.
    Property 'HTMLAudioElement' is missing in type 'Window' but required in type 'IWindow'.

but I have no idea where is the missing part. I try to follow the DialogElement implementation and I don't find any place where the Audio is missing ):

@rudywaltz I found the error. In Window.ts the property HTMLAudiolement should be renamed to HTMLAudioElement 🙂

It's failing because the interface is using the name HTMLAudioElement and it doesn't follow the interface correctly.

@capricorn86
Copy link
Owner

It seems like we got a small merge conflict as well. When that and the property is fixed I will merge it 🙂

@rudywaltz rudywaltz force-pushed the master branch 4 times, most recently from 2c0c67a to 443e5e7 Compare October 7, 2022 06:13
@rudywaltz
Copy link
Contributor Author

With this last "config resolve commit" the history is not too nice, if you need I can try rebasing and creating a new PR with more flat commit history.

@capricorn86 capricorn86 merged commit aae151b into capricorn86:master Oct 7, 2022
@capricorn86
Copy link
Owner

@rudywaltz I made a squash 🙂

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.

ReferenceError: Audio is not defined Add support for HTMLMediaElement
5 participants