Use self closing tags so sharif works with XHTML/XML mode

[theseer]

This PR adjusts the way the elements are created to support XHTML/XHTML5.

The strings past in $('<foo>') are applied to the innerHTML property and thus, when the browser is in XML mode, likely because the content has been served as application/xhtml+xml or due to the XML header, must be parse-able as XML:

XML Parsing Error: mismatched tag. Expected: </ul>.  
SyntaxError: An invalid or illegal string was specified  shariff.min.js:29

The change by this PR is simply closing all elements directly. This works fine as all other changes following are going to be added as children or attributes referencing the newly created node.

Tested in Firefox 52+53 (Linux) and Chrome 57 (Linux).

[theseer]

From #213, comment by @sehkunde

Hey, before you start this anew could you please elaborate as to why self closing tags in JS strings are necessary? Self closing <a /> or <span /> don't seem more valid to me.

The string is applied as innerHTML to an element and thus parsed by the Browser. If the Browser is in XML compliance mode, this string must be XML compliant. On the other hand, the Browser doesn't have any issues with XML-style self-closed tags in HTML mode.

Additionally I'm not aware of a use-case for XHTML to be parsed as XML in a web browser user-agent if delivered with content-type application/xhtml+xml (the <?xml> prefix doesn't trigger the xml parser in any browser or does it?)

If content is served as application/xhtml+xml the browser is set into XML compliant parsing mode - or at least should be. Technically, the actual content is supposed to start with the XML decl. header as well, but that is not a requirement.

[sehkunde]

Hi, we will merge this - I would prefer closing tags to self closing, but that's just my preference :)

I know about XML mode in browsers, but would still be interested in your use-case.

And because this piqued my interest: my reading of the jQuery documentation and some googling on Stack Overflow says:

When the parameter has a single tag (with optional closing tag or quick-closing) — $( "&lt;img /&gt;" ) or $( "<img>" ), $( "&lt;a&gt;&lt;/a&gt;" ) or $( "<a>" ) — jQuery creates the element using the native JavaScript .createElement() function.

which your error message about the <ul> line contradicts ... somethings going on there.

[theseer]

Interesting. And Confusing.

If what you said would be what is actually implemented, my change shouldn't have helped at all since both should have triggered the use of createElement.

Which actually by itself might be a problem if the HTML comes with the default namespace xmlns="http://www.w3.org/1999/xhtml". Then createElementNS should have been used. Which, at least when grep'ing over the jQuery source is not used at all in their code base. While creating the nodes technically would work, they are in the wrong namespace. I have no idea what the browser would do with that when in strict mode..

But it seems like what you found is wrong - or at least no longer how it's implemented. If I didn't misinterpret the code, jQuery uses innerHTML. I only quickly glanced over it though, so maybe I missed something.

The dom.js-Implementation shipping now also does not use createElement but innerHTML.

[sehkunde]

It's in the current documentation http://api.jquery.com/jQuery/#jQuery2, but I won't dig any further - the dom.js-Implementation is relevant and should guide what we implement here.

[glaszig]

jquery does indeed use innerHTML to leverage browser-parsing of markup. but they also employ a lot of checks and balances to be most compatible with whatever you throw at it.

even though, jquery recommends using valid markup:

To ensure cross-platform compatibility, the snippet must be well-formed. Tags that can contain other elements should be paired with a closing tag
-- http://api.jquery.com/jQuery/#jQuery2

i think @theseer's approach is the way to go. since shariff only handles the limited scope of putting buttons on a page we'd be doing well to use the most compatible markup and thereby being compatible with xhtml, jquery and our native DOMQuery.
doing all the crazy compatibility acrobatics which jquery does is outside the scope of our native DOMQuery.

regarding details of the matter, @scottschiller did a quite insightful article in 2004.

[craiq]

This approach looks not very clean to me :/
what about:
var $shareText = $("<span/>", { class: "share_text", text: self.getLocalized(service, 'shareText') });
Didn't test it. Just came to my mind, when I saw this really easy approach to add the closed tag with the content as one text line...
Docs

[glaszig]

@craiq DOMQuery does not support this syntax because it only implements functions which are actually needed by shariff.

[craiq]

forgot you changed to this ...
then another way:
var $shareText = dom("<span/>").addClass("share_text").html(self.getLocalized(service, 'shareText'));

Edit:
should be more exact: (but it's not in the documentation of domquery, just in the code)
var $shareText = dom("<span/>").addClass("share_text").text(self.getLocalized(service, 'shareText'));

[theseer]

While that indeed looks like a clean(er) approach, it would modify how the HTML is constructed rather than just fixing the XHTML/XHTML5 issue.

Maybe it's a good additional PR?

[theseer]

You probably would like to also address the $shareLink.prepend('<span class="fa ' + service.faName + '" />'); in that case.

[pmb0]

This issue should now be solved, see b230e46.

[craiq]

@pmb0 that's how it looks good :D
one little thing:
on line 166 you erased a  
thought it was there for a reason

[pmb0]

Yes, i know -- good that it's gone. :)