[PATCH 6 of 6 V2] hgweb: add actual processing of ajax-received next page content

[Alexander Plavin]

20.08.2013, 20:44, "Laurens Holst" <laurens.nospam at grauw.nl>:
> Op 20-08-13 13:21, Alexander Plavin schreef:
>
>> 20.08.2013, 13:38, "Laurens Holst" <laurens.nospam at grauw.nl>:
>>> Op 18-08-13 00:29, Alexander Plavin schreef:
>>>> # HG changeset patch # User Alexander Plavin <alexander at plav.in> # Date 1376754538 -14400 # Sat Aug 17 19:48:58 2013 +0400 # Node ID 12fcadea359e8df4d5c53cc0807acac00be38d02 # Parent 4b97b9c7034ef5d561021ce6f1f34a2f89764e7c hgweb: add actual processing of ajax-received next page content This code adds the received entries to the page. diff -r 4b97b9c7034e -r 12fcadea359e mercurial/templates/static/mercurial.js --- a/mercurial/templates/static/mercurial.js Sat Aug 17 15:35:40 2013 +0400 +++ b/mercurial/templates/static/mercurial.js Sat Aug 17 19:48:58 2013 +0400 @@ -362,6 +362,14 @@ function onstart() { }, function onsuccess(htmlText) { + var m = htmlText.match(nextHashRegex); + nextHash = m ? m[1] : null; + + var doc = docFromHTML(htmlText); + var nodes = doc.querySelector(containerSelector).children; + while (nodes.length) { + container.appendChild(nodes[0]); + }
>>>
>>> Just a note of warning, if this were XML this would be an illegal operation; you can’t just move nodes created by one document into another, it yields a WRONG_DOCUMENT_ERR. You have to call document. importNode(nodes[]) first. I guess for HTML it works though (as evidenced by your change), though I’m not sure whether this is standardised behaviour.
>>
>> As I understand from JS specification, it's standardised for HTML documents, and the 'doc' variable always represents html document. Any evidences against?
>
> Let me start off by referring to my other mail suggesting to use createDocumentFragment() which would resolve all the issues I think.
>
> Now the long explanation,
>
> “WRONG_DOCUMENT_ERR: Raised if newChild was created from a different document than the one that created this node.” [1]
>
> “This method is not allowed to move nodes between different documents. If you want to append node from a different document (for example to display results from AJAX request) you must first use document.importNode.” [2]
>
> “ Nodes from external documents should be cloned using document.importNode() (or adopted using document.adoptNode()) before they can be inserted into the current document. For more on the Node.ownerDocument issues, see the W3C DOM FAQ. Firefox doesn't currently enforce this rule (it did for a while during the development of Firefox 3, but too many sites break when this rule is enforced). We encourage Web developers to fix their code to follow this rule for improved future compatibility.” [3]
>
> Regardless of this, and the WhatWG’s “living standard” DOM standard does say in step 9 of the appendChild() implementation to “Adopt node into parent's node document.” [4], this was previously explicitly specified as not permitted.
>
> However, looking at the compatibility table at the bottom of MDN’s importNode() documentation [3], it seems to be not supported in IE8 and below. Given the WhatWG’s spec and the note in the Mozilla documentation, they probably document the de facto HTML DOM behaviour in browsers.
>
> So on the one hand I would just err on the safe side and use importNode(), or rather, adoptNode() (which doesn’t make a copy) to import those nodes before adding them. For IE8 you can add an if (document.adoptNode) check. On the other hand, you could just say that this is de facto behaviour and can be relied upon cause no browser will dare to change it.
>
> [1] http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-184E7107
> [2] https://developer.mozilla.org/en-US/docs/Web/API/Node.appendChild
> [3] https://developer.mozilla.org/en-US/docs/Web/API/document.importNode
> [4] http://dom.spec.whatwg.org/#concept-node-pre-insert

Thank you for such a thorough elaboration!

It's surprisingly hard to find compatibility information for adoptNode (I couldn't), but according to some vague discussions I think it's supported everywhere where importNode does, so we don't have to fallback to the latter if the former is not available. The only specific compatibility information I found is on SO, and it says "it is supported by all major browsers including ie 9 (personally tested on safari 5, chrome 15, firefox 7)" (about adoptNode).

Taking what you wrote into consideration, I think that adoptNode is the correct function to use here, and it's compatibility is fully OK in our case (anything against it?). Still, I was quite surprised that it's wrong (according to the specifications) to use just appendChild to move a node from another document.

>
>>> Also as htmlText does not contain a <body> tag as root element, you’re creating a document that does not have normal form (that is now standardised in HTML5, but nevertheless). Browsers may possibly insert <body> tags, etc. I think it’s better to just do createElement("div").innerHTML = ... in docFromHTML() to avoid these potential issues.
>>
>> Ehm... Why do you think that there is no <body> in htmlText? It's full html page, the one that gets generated by hgweb now.
>
> Oh, right.
>
> In that case, should you be innerHTML-ing the whole thing to documentElement? Then you’d get a double root element (<html><html></html></html>)... Maybe use outerHTML instead, if it’s supported by all browsers nowadays.
>
> Just trying to avoid weird structures, I’ve been bitten by browser handling that in different (often strange and problematic) ways in the past.

I took this implementation from https://developer.mozilla.org/en-US/docs/Web/API/DOMParser#DOMParser_HTML_extension_for_other_browsers (only a couple of lines are relevant there, others are not related to the actual document creation). So, it's most probably safe to assume this being correct.

>
> ~Laurens