Fix the getSelection method.

[treeform]

document.getSelection() actually returns a Selection object, not a cstring.

In the past this HTML method did return a DOMString but has not done this in a long time.

See this for current compatibility:
https://developer.mozilla.org/en-US/docs/Web/API/Selection#Browser_compatibility
https://caniuse.com/#search=getSelection

Nim currently returns a cstring. This kind of works because the object acts like a string since it has a .toString() method in javascript. So appears to work with a cstring but it does not totally work, here is how to break it:

// it does not stringify like a string
JSON.stringify(document.getSelection())
"{}"
// this is how a string should look like
JSON.stringify("")
""""
// it only stringify only works like a string if you convert it:
JSON.stringify(document.getSelection().toString())
""""

This means Nim’s dom API is out of sync with browsers.

A great suggestion by @dom96 we use a converter to force Selection object to behind like a string for backward compatibility.

See: #13483 (comment)

[timotheecour]

converters are a bit evil though, and I don't see how the converter will help with code written like this:

proc handleSelection(a: cstring) = ...
handleSelection(getSelection(a)) # runtime error !

how about this instead: if you use getSelection, you get a warning; if you use getSelectionObject, code is correct

proc getSelection*(d: Document): cstring {.importc: "getSelection", deprecated: "incompatible across browsers, use instead getSelectionObject".}
proc getSelectionObject*(d: Document): Selection {.importc: "getSelection".}

and if you want to support browsers that return cstring (seems like few do nowdays), you can use runtime browser detection as suggested here timotheecour#63 (comment)

[Varriount]

Would it be possible to write a test for this?

[disruptek]

Converters are idiomatic for grandfathering types like this, are they not?

[timotheecour]

Converters are idiomatic for grandfathering types like this, are they not?

like I said above, how does converter help in this case:

proc handleSelection(a: cstring) = ...
handleSelection(getSelection(a)) # runtime error ! (and no warning)

[dom96]

Looks good to me :)

[dom96]

Maybe add a {.deprecated.} pragma here (not sure if that works, but should)

[treeform]

I don't think it should be deprecated because its still a valid method that you can and will use.

I tried {.deprecated.} but now it just warns for valid calls.

[treeform]

Converters are idiomatic for grandfathering types like this, are they not?

like I said above, how does converter help in this case:

proc handleSelection(a: cstring) = ...
handleSelection(getSelection(a)) # runtime error ! (and no warning)

I have tried this code and it works just fine, it compiles without warnings and no runtime errors:

proc handleSelection(a: cstring) =
  echo a
handleSelection(document.getSelection())
document.getSelection().removeAllRanges()

[timotheecour]

this causes RT error Uncaught TypeError: a_2990031.$ is not a function
https://www.dropbox.com/s/fae2gusepi51drt/Screenshot%202020-03-31%2019.18.16.png?dl=0

see test case https://github.com/timotheecour/vitanim/blob/master/testcases/tests/t10467/readme.md

the bug is that it's inside a importcpp section so gets interpreted as such

instead do this:

# inside importcpp section:
  proc toString*(s: Selection): cstring
# outside importcpp section:
  proc `$`*(s: Selection): string = $(s.toString())
  converter toStringAux*(s: Selection): cstring = toString(s)

I tried {.deprecated.} but now it just warns for valid calls.

indeed, the following code results in deprecation messages in unrelated places (eg system.nim) (which is why I don't like converters as it causes lots of implicit conversion in places you would not expect)

converter toStringAux*(s: Selection): cstring {.deprecated.} = toString(s)

Maybe you could hide the converter under when defined (nimDeprecatedJsDomSelectionConverter): ...