BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
856 stars 130 forks source link

html comments not working in tables #452

Closed johan-ohrn closed 3 years ago

johan-ohrn commented 3 years ago

I'm trying to insert html comments in my generated html for better readability but I'm getting this error. Could you please have a look at the error by running this short sample? (jsfiddle - check browser console)

var tmpl = '<table><tbody>{^{for}}{{/for}}</tbody><!-- comment --></table>';
$.templates(tmpl).link("#ph");
---
jsviews.js:4690 Uncaught TypeError: elem[((intermediate value)(intermediate value)(intermediate value) + "Attribute")] is not a function
    at setDefer (jsviews.js:4690)
    at processViewInfos (jsviews.js:5527)
    at dataLink (jsviews.js:5668)
    at View.viewLink [as link] (jsviews.js:5893)
    at $link (jsviews.js:5301)
    at Function.tmplLink [as link] (jsviews.js:5211)
    at ?editor_console=true:117
setDefer @ jsviews.js:4690
processViewInfos @ jsviews.js:5527
dataLink @ jsviews.js:5668
viewLink @ jsviews.js:5893
$link @ jsviews.js:5301
tmplLink @ jsviews.js:5211
(anonymous) @ ?editor_console=true:117

If I surround the comment with other html tags (in this case <template> tags) I don't get the error. Also note that the linked {^{for}} tag is required to get this error.

var tmpl = '<table><tbody>{^{for}}{{/for}}</tbody><template></template><!-- comment --><template></template></table>';
$.templates(tmpl).link("#ph");

I've tested with the latest version of jsviews.

BorisMoore commented 3 years ago

Thanks Johan. Here is an update which should fix that issue:

jsViews11a.zip

The change is line 5517 and 5512:

while (deferChar = deferPath.charAt(j--)) {
    // Use the "+" and"-" characters to navigate the path back to the original parent node where the deferred bindings ocurred
    if (deferChar === "+") {
        if (deferPath.charAt(j) === "-") {
            j--;
            targetParent = targetParent.previousElementSibling; // IE9 or later only
        } else {
            targetParent = targetParent.parentNode;
        }
    } else {
        targetParent = targetParent.lastElementChild; // IE9 or later only
    }

But <IE9 did not support those APIs. I guess at this point we can remove support for IE8 without concern.... Otherwise we would need to add a polyfill, or equivalent...

Let me know if it works for you...

johan-ohrn commented 3 years ago

Works like a charm, thanks!

At this point one would hope IE8 is not a concern. I think you can do as you did with unicode support and place polyfills in a separate file to prevent cluttering the main js file(s). I.e. jsviews.ie8.js, jsviews.ie9.js... That would give you more freedom to use more modern apis and remain backwards compatible. I note that there are polyfills on MDN: https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/lastElementChild https://developer.mozilla.org/en-US/docs/Web/API/NonDocumentTypeChildNode/previousElementSibling

Interestingly its stated that previousElementSibling is only partially supported for IE9.

BorisMoore commented 3 years ago

I have been considering the jsviews.ie8.js idea, but I'm inclined to limit backward compat goals to IE9, not IE8.

Currently all the code should be IE9 compatible, including the new code from this fix. (My tests all pass in IE9). OTOH, there are a few features (such as sort and filter on the {{for}} tag) which already fail in IE8. If I provide a polyfill, then I would need to commit to testing in IE8 with the polyfill included. I'd prefer not to add to the testing matrix. (IE8 has a lot of inconsistent white-space rendering that makes testing more time consuming.)

That said, when users encounter the fact that Array.map or element.previousElementSibling are undefined in IE8, they are clearly free to add a polyfill themselves (and would need to test it in their environment....)

BorisMoore commented 3 years ago

This has been resolved in v1.0.11.