Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Knockout cannot process component template correctly if it is loaded by element identifier (IE 11). #1836

Closed
ghost opened this issue Jul 30, 2015 · 10 comments

Comments

@ghost
Copy link

ghost commented Jul 30, 2015

The following code works fine in Firefox, Chrome, Opera, but does not work in IE (at least in IE 11):

<template id="common-account">
  <p>First name: <input data-bind="value: firstName" /></p>
  <p>Last name: <input data-bind="value: lastName" /></p>
  <h2>Hello, <span data-bind="text: fullName"> </span>!</h2>
</template>
var ViewModel = function(first, last) {
    this.firstName = ko.observable(first);
    this.lastName = ko.observable(last);

    this.fullName = ko.pureComputed(function() {
        return this.firstName() + " " + this.lastName();
    }, this);
};

ko.components.register("common-account", {
    template: { element: "common-account" },
    viewModel: {
        createViewModel: function (params, componentInfo) {
            var viewModel = new ViewModel("User", "Test");

            return viewModel;
        }
    },
    synchronous: true
});

ko.applyBindings();

To explain solution I have created jsfiddle: http://jsfiddle.net/qp49uz0v/16/
It raises exception that firstName is undefined.

IE works fine as well only if template is specified as string (like it is here: http://jsfiddle.net/47e2gpqq/4/).

@IanYates
Copy link

Interesting problem. I can confirm it happens for me too (IE 11 on Windows 8.1)
I also loaded up a copy of Win 10 and tried it in Edge. Same issue.

@brianmhunt - I wonder if this is what's causing grief for the new KO docs site in IE? I recall reading this morning in your blog post that you concatenate all of the templates into a single file. Presumably they're referenced by their id in a manner similar to that described in this issue.

@brianmhunt
Copy link
Member

@IanYates Ahh yes, that may indeed be the issue.

@maxim-boligatov Does the problem exhibit in IE when you rewrite the <template id='..'></template> as a <script id='...' type='text/x-ko></script>`? Older WebKit may need this rewrite too.

If that does work, then one can do a javascript polyfill for the <template> tag or a relatively simple jQuery equivalent.

@IanYates
Copy link

@brianmhunt - just tried script tags. They work a treat - http://jsfiddle.net/qp49uz0v/18/ :)

@brianmhunt
Copy link
Member

Thanks @IanYates ; looks like <template> is not supported in any version of IE, per http://caniuse.com/#feat=template

So it looks like this is not a Knockout issue per se, but an IE problem. We all have to fall back on a polyfill for IE support.

This being an IE issue I'm not sure what we could be doing better in KO here. @maxim-boligatov if you think there's still something KO could be doing better here please let me know. In the mean time I'll close this, but happy to reopen if there's more to discuss or something else to be done here.

Thanks and cheers.

@ghost
Copy link
Author

ghost commented Jul 30, 2015

I have posted previous comment too early. So I wrote new one.
Idea is the same bind template as string. So new jsfiddle url: http://jsfiddle.net/qp49uz0v/26/
I think knockout can easy detect all templates if it is IE, cache inner html and drop element itself.

<template id="common-account">
  <p>First name: <input data-bind="value: firstName" /></p>
  <p>Last name: <input data-bind="value: lastName" /></p>
  <h2>Hello, <span data-bind="text: fullName"> </span>!</h2>
</template>

<common-account></common-account>
var ViewModel = function(first, last) {
    this.firstName = ko.observable(first);
    this.lastName = ko.observable(last);

    this.fullName = ko.pureComputed(function() {
        return this.firstName() + " " + this.lastName();
    }, this);
};

var trick = document.getElementById('common-account');
var template = trick.innerHTML;
trick.parentNode.removeChild(trick);

ko.components.register("common-account", {
    template: template,
    viewModel: {
        createViewModel: function (params, componentInfo) {
            var viewModel = new ViewModel("User", "Trick");

            return viewModel;
        }
    },
    synchronous: true
});

ko.applyBindings();

@brianmhunt
Copy link
Member

Thanks @maxim-boligatov & @IanYates – I think I see this now. Sorry, it takes me a minute. 😁

I am guessing that at components/defaultLoader.js#L168-L170 we'd want something like:

                if (isDocumentFragment(elemInstance.content)) {
                    return ko.utils.cloneNodes(elemInstance.content.childNodes);
                }
                //  Fallback where <template> tag is not natively supported. See #1836.
                return ko.utils.parseHtmlFragment(elemInstance.innerHTML);

I really don't know what IE is doing, so I'm only speculating at a patch here. The fallback of return ko.utils.cloneNodes(elemInstance.childNodes); is evidently not working.

@mbest Do you think the potential patch here could be this simple? Any insight into what's going on?

@mbest
Copy link
Member

mbest commented Jul 30, 2015

@brianmhunt, I'll take a look.

@brianmhunt
Copy link
Member

@maxim-boligatov @IanYates What does the following yield on IE?

    function isDocumentFragment(obj) {
        if (window['DocumentFragment']) {
            return obj instanceof DocumentFragment;
        } else {
            return obj && obj.nodeType === 11;
        }
    }

where obj is the document.getElementById('common-account')?

@chessels
Copy link

chessels commented Aug 4, 2015

This is already fixed. See issue 1783.

@mbest
Copy link
Member

mbest commented Aug 4, 2015

Thanks @chessels for reminding us. We need to get 3.4.0 out the door!

@mbest mbest closed this as completed Aug 4, 2015
@mbest mbest removed this from the 3.4.0 milestone Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants