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

Nodes stamped by dom-if are duplicated when imported-template is reattached to DOM #26

Open
warpech opened this issue Jan 17, 2017 · 14 comments

Comments

@warpech
Copy link
Contributor

warpech commented Jan 17, 2017

This is a continuation of to Starcounter/starcounter-include#17, because after some research, I have managed to further isolate the problem to imported-template.

The problem is that nodes stamped by dom-if are duplicated when the parent node of imported-template is detached and then attached to DOM

I have written a test that fails in imported-template. I will commit it to a branch.

I think that the problem is between imported-template and Polymer, because using only Polymer this works as desired: https://jsbin.com/pesece/1/edit?html,js,output)

warpech added a commit that referenced this issue Jan 17, 2017
@warpech
Copy link
Contributor Author

warpech commented Jan 17, 2017

I did not manage to fix this, though I have isolated the problem in a single test. See the above commit.

This causes a problem in the Website app, meaning that it blocks the Launcher roadmap (https://github.com/StarcounterPrefabs/Launcher/issues/261, point 9)

@miyconst could you please take a look? Maybe a new pair of eyes can fix it.

screen shot 2017-01-17 at 18 47 52

@warpech
Copy link
Contributor Author

warpech commented Feb 13, 2017

I think I know what the problem is.

imported-template uses juicy-html's clear method, which only removes stampedNodes, but not scopedNodes nor scopelessNodes.

Overloading this method in imported-template using the following code fixes the problem.

I have made some test last month (commit e539d12) without a solution to the problem. Now I think I have the solution for the problem (see the snippet below). I am just not sure if it fixes the same problem or some other problem, but I am sure it is needed ;) Definitely the quality of it can be improved.

@tomalec, would you have time to check if this can solve the problem?

        var clear = JuicyHTMLElement.prototype.clear;
        ImportedTemplatePrototype.clear = function () {
            var childNo,
                scopedNodes = this.scopedNodes,
                len = scopedNodes && scopedNodes.length || 0;

            for (childNo = 0; childNo < len; childNo++) {
                clear.call({
                  stampedNodes: scopedNodes[childNo]
                });
            }
            this.scopedNodes = [];

            clear.call({
              stampedNodes: this.scopelessNodes
            });
            this.scopelessNodes = [];

            // inline HTML
            if (!this.scopedNodes && !this.scopelessNodes) {
              clear.call({
                stampedNodes: this.stampedNodes
              });
              this.stampedNodes = null;
            }
        }

Then, you also need to change:

                xhtml.scopedNodes = [];
                xhtml.scopelessNodes = [];
                xhtml.clear();

to:

                xhtml.clear();

@tomalec
Copy link
Member

tomalec commented Feb 13, 2017

Nic catch! I'll check it tomorrow morning

@tomalec tomalec self-assigned this Feb 13, 2017
@tomalec
Copy link
Member

tomalec commented Feb 14, 2017

I think that the problem is between imported-template and Polymer, because using only Polymer this works as desired: https://jsbin.com/pesece/1/edit?html,js,output)

It seems your re-pro with only polymer, was checkign wrong thing. Take a look here
https://jsbin.com/xiwuhogeho/1/edit?html,js,console,output

@tomalec
Copy link
Member

tomalec commented Feb 14, 2017

I don't think your snippet is needed and could help somehow, as at https://github.com/Juicy/imported-template/blob/master/imported-template.html#L103 we are making stampedNodes contain all child nodes of stamped fragment therefore it should contain both scoped and scopeless as well.

Also it does not make tests https://github.com/Juicy/imported-template/tree/reattached-fix/test/use-cases/dom-manipulation pass.

@warpech
Copy link
Contributor Author

warpech commented Feb 24, 2017

Yeah, I just was able to reproduce the original problem by running Website + People + SignIn.

My snippet from #26 (comment) indeed does not help with that problem.

@tomalec any idea how to fix the OP?

@tomalec
Copy link
Member

tomalec commented Feb 24, 2017

Ok, now I can reproduce it, I'll dig deeper

@tomalec
Copy link
Member

tomalec commented Feb 24, 2017

I have reduced it a little bit:
Website + People with two simple HTMLs + SignIn

So, the suspects are : imported-template, Polymer, People app

still investigating

@tomalec
Copy link
Member

tomalec commented Feb 24, 2017

I have found something that could cause it https://github.com/StarcounterPrefabs/Website/issues/46, however imported-template should handle it anyway

@tomalec
Copy link
Member

tomalec commented Feb 24, 2017

After some investigation I would bet it's related to this issue Polymer/polymer#3682
And I would really like not to try fixing it or working it around in yet another place before Polymer update.

I suspect fixing https://github.com/StarcounterPrefabs/Website/issues/46 should hide this problem for some time.

@warpech WDYT?

@un-tone
Copy link
Member

un-tone commented Mar 3, 2017

Fixing https://github.com/StarcounterPrefabs/Website/issues/46 does not hide the problem.

@tomalec
Copy link
Member

tomalec commented Mar 6, 2017

Could you provide a build with that fix, so I could investigate further?

@un-tone
Copy link
Member

un-tone commented Mar 6, 2017

@tomalec this PR https://github.com/StarcounterSamples/Website/pull/53 hides the problem

@warpech
Copy link
Contributor Author

warpech commented Oct 3, 2017

Get back to this in Starcounter 2.4

@tomalec tomalec removed their assignment Jan 6, 2018
@warpech warpech added this to the imported-template stability milestone Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants