tmedwards / sugarcube-2

SugarCube is a free (gratis and libre) story format for Twine/Twee.
https://www.motoslave.net/sugarcube/2/
BSD 2-Clause "Simplified" License
185 stars 42 forks source link

Print macros force toString() on Element instances #87

Closed Akjosch closed 3 years ago

Akjosch commented 4 years ago

When trying to output DOM elements via print macros (no matter which of the three), they run through the toPrintOrDefault() helper function, which treats them like any other object - that is, it transforms them into their string representation (typically something like "[object HTMLDivElement]") - and then through the Wikifier instead of simply attaching them to the output as-is.

For now, most of the time we can use <<= _variable.outerHTML>>, but that's not very robust (what if the HTML contains something the SugarCube Wikifier will pick up?) and kinda counter-intuitive.

tmedwards commented 3 years ago

Sorry for the delayed response.

First. Let me say that <<print>> probably should do something more intelligent than it currently is when passed an element object, so I definitely agree with you there. That said….

I'd first ask why <<print>>-ing an element object—with the implied, though I'd hope obvious, context being as a string—seems like a good idea in the first place? Rather than simply appending it to the current render buffer, as the most obvious counter example.

Beyond that, there's no obviously "right" way to go about printing an element. I mean, we could output: the element object itself as-is, the value of its .outerHTML, the value of its .textContent. All of those are more-or-less reasonable actions in the context of <<print>>—though the latter two make the most sense in that context.

Maybe if I had more context on what you were attempting to do this would make more sense to me.

Akjosch commented 3 years ago

The context was a simple bit of a widget which printed an "avatar image", and allowed for that to be a string (= URL to the image), an Element (= some more complex structure, for a paperdoll system) or a function which returns one of these. The actual code looked like this:

<<set _avatar = (typeof _element.avatar === "function" ? _element.avatar() : _element.avatar)>>
<<if _avatar instanceof Element>>
    <<= _avatar>>
<<else>>
    <img @src='_avatar'>
<</if>>

Obviously, in this specific case, changing the third line into ...

    <<= _avatar.outerHTML>>

... worked fine. But if that Element instance had event handlers attached, this would break them, and it would be nice if we had some way to simply attach such an instance to the current page during the initial parsing. The "print" macros, in preference to another (custom) macro, are just the obvious place, since outputting the toString() result of those objects makes little sense.

tmedwards commented 3 years ago

And I sort of get that <<print>> seems the obvious go-to here, except that <<print>> is explicitly about turning things into strings—especially the <<->> variant. That it tries to print string representations of commonly encountered objects should not be taken as proof that its function is to print objects, as that's more about allowing users to ghetto debug their variables.

Again, I'm wondering why you didn't simply append it to the current render buffer via <<script>>. For a vanilla example:

<<if _avatar instanceof Element>>
    <<script>>output.appendChild(temporary().avatar)<</script>>
<<else>>

Alternatively. At the point when you're dipping into the JavaScript side of the pool as much as the markup/macro side, my general advice is to embrace the JavaScript. I mean, there doesn't seem to be a compelling reason not use a <<script>> for your entire example.

Regardless. I'm going to make attempting to print elements work better than it does now, but it's likely to be by simply printing the value of .outerHTML. When you need to output constructed DOM elements as-is, just use <<script>> or a custom macro.