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

Fix type of HtmlElement.ofElement #60

Closed
TheSpyder opened this issue Dec 6, 2021 · 6 comments · Fixed by #71
Closed

Fix type of HtmlElement.ofElement #60

TheSpyder opened this issue Dec 6, 2021 · 6 comments · Fixed by #71
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@TheSpyder
Copy link
Owner

I totally forgot we inherited this bug:
https://github.com/tinymce/rescript-webapi/blob/18d9d553201d575fa8e194d3370298b556b28747/src/Webapi/Dom/Webapi__Dom__HtmlElement.res#L8

The type should be let ofElement: Dom.element => option(t_htmlElement) but currently it's Dom.element => option<Dom.htmlElement>. This concrete type breaks modules that inherit from HtmlElement, e.g. reasonml-community/bs-webapi-incubator#205

This happened when bs-webapi was changed to use safer instanceof checks rather than the previous way of checking if a HTML attribute existed:
https://github.com/reasonml-community/bs-webapi-incubator/pull/182/files?file-filters%5B%5D=.re#diff-f6590c311eac02db4a8591dfe7b1a4c232207269ed2d71105f8407ad4c5ba34dL13-R4

There have been a number of bugs with this change - we perhaps need to rethink it.

@TheSpyder TheSpyder added bug Something isn't working help wanted Extra attention is needed labels Dec 6, 2021
@TheSpyder
Copy link
Owner Author

I have a branch that should fix this. I need to do some more testing before I open a PR.
https://github.com/tinymce/rescript-webapi/compare/fix_ofElement

@spocke
Copy link
Collaborator

spocke commented Dec 7, 2021

It seems that IE doesn't support function.name https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name not sure if we care about IE 11 for these bindings?
What was the bugs with the instanceof method?

@TheSpyder
Copy link
Owner Author

TheSpyder commented Dec 7, 2021

Ah, damn. I had a version that included a fallback but then I saw prototype.constructor has worked since ie8.

This is a similar change that was made in TinyMCE recently - turns out there was an edge case where creating an element in the outer window and attaching it in an iframe breaks all instanceof checks.

@spocke
Copy link
Collaborator

spocke commented Dec 7, 2021

But wouldn't the element.defaultView.HTMLElement fix that case?

@TheSpyder
Copy link
Owner Author

There’s a TinyMCE test that failed with the old code. I’ll post the link when I get to my desk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants