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 ofElement #71

Merged
merged 3 commits into from
Dec 28, 2021
Merged

Fix ofElement #71

merged 3 commits into from
Dec 28, 2021

Conversation

TheSpyder
Copy link
Owner

Fixes #60

The main fix is using Obj.magic to correct the input type for the ofElement function, but I also took the opportunity to shorten asHtmlElement.

This implementation uses techniques similar to what we do in TinyMCE combined with an unsubmitted patch I made that is shorter but doesn't support IE11.

@TheSpyder TheSpyder requested a review from spocke as a code owner December 26, 2021 23:43
src/Webapi/Dom/Webapi__Dom__HtmlElement.res Show resolved Hide resolved
*/
let asHtmlElement = %raw(`
function(element) {
if ((window.constructor.name !== undefined && /^HTML\w*Element$/.test(element.constructor.name))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately this doesn't work for custom elements like web components since they can be named anything. I wonder if check is even possible to get right for every scenario. Since the element might have been created by some other window/document like the tinymce case or it might be a custom element that extends either element or htmlelement and it can also in be from other documents. We can't check for specific properties that one expects to only exist on a HTMLElement since className for example exists on svg elements that are instances of Element not HTMLElement and custom elements could for example extend a Element and provide any HTMLElement property that we check for.

So what options do we have:

  1. Remove the check all together making this just a weak magic cast.
  2. Let people fallback to using unsafeAsHtmlElement for the custom element case might be very annoying depending on app.
  3. Keep the old check that works for the same document/window combination.
  4. Do a mixture of both that still doesn't cover the case if it's a custom element defined in another document.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This method exists for two reasons:

  • Calling HtmlElement binding methods on an Element
  • As a basic safety net for the ofElement methods used by every module that includes HtmlElement bindings

Neither of which, imo, need to care about custom elements. This new code should be just as reliable as the check was before but also work across document/window boundaries. I just wanted to make a positive step forward, not fix every possible edge case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It thought it existed to safely cast Element to HtmlElement. But I guess it would be a weird scenario that some one makes a custom element that inherits from Element that is called HTML..Element. I guess if you want to toggle a class on all your custom elements that inherit from HTMLElement you just need to use unsafe casting since we can never for every scenario make that check safe.
I might be to paranoid about type safety. Just trying to think how this would work in some custom element heavy app like a web components app.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking into it some more toggling classes should work since both element and html element seems to have that property however things like contentEditable or styles will now no longer work if the foo-item and bar-item are web components. The old check worked fine for all apps that doesn't use iframes I think our use case is the odd ball here so not sure we should potentially break existing apps just to fix issues we have with elements in iframes.

document
->Document.querySelectorAll("foo-item,bar-item")
->NodeList.toArray
->Belt.Array.forEach(node => {
  node
  ->Element.ofNode
  ->Belt.Option.flatMap(HtmlElement.ofElement)
  ->Belt.Option.forEach(HtmlElement.setContentEditable(_, True))
})

I also found an issue with HtmlElement.ofNode that just doesn't do any checking. #75 it just checks if it's a element then downcasts that to a HtmlElement.

@TheSpyder TheSpyder merged commit 44a621d into main Dec 28, 2021
@TheSpyder TheSpyder deleted the fix_ofElement branch December 28, 2021 05:28
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.

Fix type of HtmlElement.ofElement
3 participants