search

TheSpyder / rescript-webapi

ReScript bindings to the DOM and other Web APIs
http://tinymce.github.io/rescript-webapi/api/Webapi/
Other
149 stars 36 forks source link

Fix type of HtmlElement.ofElement #60

Closed TheSpyder closed 2 years ago

TheSpyder commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

TheSpyder commented 2 years ago

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

TheSpyder commented 2 years ago

https://github.com/tinymce/tinymce/blob/715f59c3a8b2bb35d2a158902a1b245987185b17/modules/sand/src/test/ts/browser/HtmlElementTest.ts#L34-L51