search

slamdata / purescript-markdown-halogen

A Halogen rendering library for purescript-markdown
Apache License 2.0
10 stars 9 forks source link

textBoxTypeName returns options that are not valid for IE #11

Closed jonsterling closed 9 years ago

jonsterling commented 9 years ago

Follow my observations here, it seems as though an exception is thrown in Internet Explorer if an input element's type attribute is set to an unsupported value. Now, Internet Explorer doesn't support almost any of the various input types, so we need to be very cautious.

Presumably, this throws a wrench in the entire rendering operation, which is what caused the SlamData Notebook view to appear empty.

Here's an example file that you can use to reproduce this issue in Internet Explorer:

<html>
  <body>
    Hold on to your (boots):
    <script type="text/javascript">
      var el = document.createElement("input");
      el["type"] = "time";
      document.body.appendChild(el);
    </script>
  </body>
</html>

When the script is executed, an error is thrown (Invalid argument.) and the element of course not displayed.

Maybe we should adjust textBoxTypeName to observe the current browser and only fall back to text if it detects Internet Explorer?

jdegoes commented 9 years ago

Arguably this could be solved in Halogen by try / catching the assignment of input types and defaulting to text. I'm honestly not sure where it belongs. But browser detection is the last resort, ideally you try something and if it doesn't work, deal with the failure that way (more robust than detecting browser versions).

garyb commented 9 years ago

Arguably this could be solved in Halogen by try / catching the assignment of input types and defaulting to text.

That's going to be quite difficult to do actually, as it's virtual-dom that will create the element during the patching process, so explicitly handling that one thing may be problematic. It may be solvable with a thunk or widget (in the virtual-dom sense) perhaps...

I agree on attempting that approach though, it does seem like the right thing to do.

jonsterling commented 9 years ago

Right—I'm still not deeply familiar with how the virtual-dom stuff works here, but it seems like it would be very difficult to put the try-catch logic in the right place, since at whatever point it would be possible to do so, we won't know what sensible default to substitute ("text"), right?

jdegoes commented 9 years ago

This is something I imagine would be hidden inside the JS part of Halogen that deals with virtual-dom. Though @garyb knows this a lot better than me. One could consider it a bug that Halogen's patching process can fail due to browser quirks like this -- it'd certainly be nice if we had the guarantee that patching couldn't bomb due to browsers rejecting certain attribute assignments. At the very least such low-level patching errors should be catchable and there should be some way of dealing with them to aid diagnosing issues like this in the future (surely IE 11's type attribute on input is not the only case like this).

jonsterling commented 9 years ago

@jdegoes Yeah, I'll have to look into the code a bit more—but I also would like to suggest that maybe bailing out is the right thing to do in such cases, since it indicates a bug in the code.

There are likely numerous other cases where browser incompatibilities will cause exceptions to be thrown in addition to this type thing, as you note—and I think trying to play catchup by means of a big block of special cases in the low-level code is a losing battle. Additionally, there are probably cases where the try-catch approach won't even suffice—namely, any case where there is no single default that works in each browser. For this reason, it seems plausible to me that detecting the browser type is inevitable in the long run...

If we are intent to solve this at the level of Halogen, maybe an alternative would be to separately provide a function Html -> Html, which traverses the syntax tree and "fixes" stuff like this based on an observation of the browser. This has the benefit of being more modular and transparent... It does have the disadvantage of resulting in another traversal of the tree, of course.

My preferred alternative, though, would be to simply state a law that anything constructed by purescript-markdown-halogen shall be compatible with {...} browsers, and then ensure this in the implementation, since we cannot hope to address all possible problems directly in the purescript-halogen library—but we can do so for this library, because it is the library which determines in every case what HTML elements are constructed.

garyb commented 9 years ago

I must admit I'm a little surprised it does throw an exception - I think our usages are all technically valid: http://www.w3.org/TR/html5/forms.html#attr-input-type

Aside from DateTime actually, that one should probably be rendered as a separate date and time fields to be standards compliant.

As for throwing exceptions, this is also not a standard behaviour for the type attribute as far as I can tell from reading the spec, so yes, there may be some other cases where invalid values in Halogen can cause this, but it's not obvious where they may occur as it's an implementation specific behaviour.

jdegoes commented 9 years ago

For this reason, it seems plausible to me that detecting the browser type is inevitable in the long run...

Rather than browser detect, I suggest feature detection since it's more robust. e.g. you'd have some boolean flag in Eff or Aff saying whether or not the browser supports type = date, for example.

Aside from DateTime actually, that one should probably be rendered as a separate date and time fields to be standards compliant.

:+1:

As for throwing exceptions, this is also not a standard behaviour for the type attribute as far as I can tell from reading the spec

The problem is really that, it's basically assumed in Halogen the patching process will succeed. So there's no way to deal with failure when and if it happens. That could be a hook of some type.

jonsterling commented 9 years ago

I like the feature detection idea a lot better than browser detection, :+1:. Then, it can be dealt with separately how to populate that information (some strategies may be better than others, but we shouldn't fix in advance which strategy will be used in an application).

Regarding having a hook for patching failure, I'm not really opposed to it—but is there any case where handling this error would not be hiding an actual bug in the code?

jdegoes commented 9 years ago

The issue is not so much hiding a bug, but user experience: it's extremely unpleasant to have the whole UI disappear without a trace of what's going wrong, and no errors in the console.

Errors need to be catchable so we can deal with them gracefully, e.g. showing a dialog to the user with the error message which allows submitting the error directly to SlamData.

jonsterling commented 9 years ago

@jdegoes Ah, I see, that sounds right. I was a bit worried about the idea of catching the error and trying to resolve it in-code, but the hook seems perfectly justifiable as a way to avoid confusing the user, and allow us to collect better bug reports.

jdegoes commented 9 years ago

Yeah, I'm not a big fan of "catch and ignore". The best thing we can do for unexpected errors is to encourage the user to submit the error, together with browser environment information (which is easy for us to collect, and more reliable than self-reporting), and then give the user the option to "continue" or "reload" (though for an error like this when the UI disappears, only option will be reloading).

jonsterling commented 9 years ago

Cool, sounds good. So, I think that we can deal with this as two separate issues; the immediate one is to fix this code so that it doesn't give IE a reason to throw an exception, and separately, we should probably look into what is necessary to provide a hook for such unexpected errors.

jdegoes commented 9 years ago

:+1:

jonsterling commented 9 years ago

How do people feel about these changes? https://github.com/slamdata/purescript-markdown-halogen/commit/8d9f38aca93f55d1d40338776ea10f8426032dd8

garyb commented 9 years ago

Looks like a good way of modelling it to me :+1:

jonsterling commented 9 years ago

@garyb thanks for the feedback! I'll open a PR.