-
Notifications
You must be signed in to change notification settings - Fork 37
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
Fix ofElement
#71
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
15 changes: 15 additions & 0 deletions
15
lib/js/tests/Webapi/Dom/Webapi__Dom__HtmlInputElement__test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
'use strict'; | ||
|
||
var Curry = require("rescript/lib/js/curry.js"); | ||
var Caml_option = require("rescript/lib/js/caml_option.js"); | ||
var Webapi__Dom__HtmlInputElement = require("../../../src/Webapi/Dom/Webapi__Dom__HtmlInputElement.js"); | ||
|
||
var input = Curry._1(Webapi__Dom__HtmlInputElement.ofElement, document.body); | ||
|
||
if (input !== undefined) { | ||
var input$1 = Caml_option.valFromOption(input); | ||
input$1.focus(); | ||
input$1.select(); | ||
} | ||
|
||
/* input Not a pure module */ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
open Webapi.Dom | ||
|
||
// verify `ofElement` returns the correct type | ||
@val external element: Element.t = "document.body" | ||
|
||
switch (element->HtmlInputElement.ofElement) { | ||
| Some(input) => | ||
HtmlInputElement.focus(input); | ||
HtmlInputElement.select(input); | ||
| None => () | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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:
HtmlElement
binding methods on anElement
ofElement
methods used by every module that includesHtmlElement
bindingsNeither 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.