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

components v1 native implementation - inner components problem #615

Closed
tnikolai2 opened this issue Dec 9, 2016 · 18 comments
Closed

components v1 native implementation - inner components problem #615

tnikolai2 opened this issue Dec 9, 2016 · 18 comments

Comments

@tnikolai2
Copy link

tnikolai2 commented Dec 9, 2016

for example :
HTML: '<tag1>aa<tag2></tag2>bb</tag1>'

in tag1.connectedCallback (at chrome native v1 implementation ) this.innerHTML contain only "aa"
and this.childNodes contain one text node "aa". '<tag2></tag2>bb</tag1>' appended only after component tag1 renderered.

If i do: tag1.connectedCallback(){ this.innerHTML='xx';}
result is '<tag1>xx<tag2></tag2>bb</tag1>'

So at chrome native v1 implementation at connectedCallback
if component contain nested components, innerHTML and childNodes contain only data before first inner component occurence.

at v0 and non-native v1 implementation innerHTML all time contain correct value 'aa<tag2></tag2>bb'

What can do with this problem?

Sample page:

<pre>
	class tag1 extends HTMLElement{
		connectedCallback(){console.log(this.innerHTML);  	this.innerHTML='xx' }
	}
	customElements.define('tag-1', tag1);
	customElements.define('tag-2', HTMLElement);
</pre>

First Variant(correct):  document.write('&lt;tag-1>aa&lt;tag-2> TAG2 &lt;/tag-2>bb&lt;/tag-1>')<br>
<script>
	class tag1 extends HTMLElement{
		connectedCallback(){console.log("innerHTML: "+this.innerHTML);  	this.innerHTML='xx' }
	}
	customElements.define('tag-1', tag1);
	customElements.define('tag-2', HTMLElement);
	
	document.write('<tag-1>aa<tag-2> TAG2 </tag-2>bb</tag-1>');
</script>

<br><br>Second Variant(incorrect)- in html: &lt;tag-1>aa&lt;tag-2> TAG2 &lt;/tag-2>bb&lt;/tag-1><br>
<tag-1>aa<tag-2> TAG2 </tag-2>bb</tag-1>

<br/><br/> <br/>  In second variant property innerHTML inside connectedCallback contains only data before first occurence of nested custom element.
Remainder appended after element will be rendered in any case, even if assign innerHTML empty string.
@tnikolai2
Copy link
Author

Ispend so many time((.
This problem only if component placed statically. If contend places dynamically in js all work as need.

@TakayoshiKochi
Copy link
Member

@tnikolai2 could you provide your test cases via jsbin.com (or your favorite service) so people can reproduce the case and understand easily?

If it is specific to Chrome, you can file a bug at http://crbug.com .

Usually people get confused when looking at .innerHTML from constructor, but this seems not the case, as you are talking about connectedCallback().

For constructor, you can refer to this document:
https://slack-files.com/T03PF4L4C-F5L02PUPR-dba68f695a

@tnikolai2
Copy link
Author

now document.write('aa TAG2 bb'); also incorrect
and give result 'xxaa TAG2 bb'

qq.innerHTML='aa TAG2 bb'; Only this third variant give correct result 'xx' .. This compound tag for example cannot implement with web components v1.

@TakayoshiKochi
Copy link
Member

I don't understand what you are talking about "incorrect".

Please provide live test cases about "variants" you are talking about (using http://jsbin.com or something) that we can verify, and what is your expectation and what is not your expectation.
Which browser and version are you testing against?

@tnikolai2
Copy link
Author

tnikolai2 commented Jul 10, 2017

@annevk
Copy link
Collaborator

annevk commented Jul 10, 2017

That's the intended behavior. You can't assume a particular tree at construction time or insertion time as custom elements can also be created via document.createElement("tag-1") or new tag1.

@tnikolai2
Copy link
Author

but with innerHTML connectedCallback can access and modify children content. With document.write it also worked before.

@annevk
Copy link
Collaborator

annevk commented Jul 10, 2017

And it still can. But you have to understand that the children might not yet been inserted.

@TakayoshiKochi
Copy link
Member

I'm just seeing error message on Chrome with the jsbin demo,

"error"
"TypeError: Illegal constructor
    at <anonymous>:9:11
    at https://static.jsbin.com/js/prod/runner-4.0.4.min.js:1:13850
    at https://static.jsbin.com/js/prod/runner-4.0.4.min.js:1:10792"
...

So I'm not completely sure the problem you are encountering, but as I understand @annevk's comment, the behavior you are seeing is working as expected.

When HTML parser parses a custom element, it connects the parsed custom element after calling its constructor, but before connecting all the children which are not yet parsed. On the other hand, if all the custom elements are parsed but not upgraded (e.g. you put the custom elements in HTML before script that defines the custom element runs), when they are upgraded they can see their children, as they are already parsed and all connected (.innerHTML also parses, inserts, and then upgrades them, so you can see children).

Closing this as working as expected.

@tnikolai2
Copy link
Author

tnikolai2 commented Jul 11, 2017

http://jsbin.com/fopozupofa/1/edit?html,console,output

<script> div.innerHTML='aa TAG2 bb'; //xx document.write('
Four: aa TAG2 bb'); //xxaa TAG2 bb </script>

Variant 4 runs after 3, where all was upgraded, but it same as 1 and 2 - very strange

@tnikolai2
Copy link
Author

tnikolai2 commented Jul 11, 2017 via email

@TakayoshiKochi
Copy link
Member

Thanks for fixing the jsbin demo. I can see your problem now.

So when HTML parser sees the <tag-1>..., the following sequence happens in Chrome:

  1. recognize opening <tag-1>
  2. construct tag1
  3. connect <tag-1> to its parent (connectedCallback() is called - adds xx as innerHTML)
  4. recognize aa and append to <tag-1>
  5. recognize <tag-2> TAG2 </tag-2> and append to <tag-1>
  6. recognize bb and append to <tag-1>
  7. recognize closing </tag-1>

which results in showing xxaa TAG2 bb, and you describe as incorrect.

For .innerHTML case, roughly the following happens

1 <tag-1>aa<tag-2> TAG2 </tag-2></tag-1> are parsed by fragment parser, which does not synchronously construct custom elements (thus <tag-1> and <tag-2> remain HTMLElement)
2. construct DOM fragment (<tag-1> as root)
3. append the fragment to its parent
4. upgrade <tag-1>, <tag-2> in this order - constructor() and connectedCallback() are called

In this case, <tag-1>'s children are all available when its connectedCallback() is called, thus .innerHTML = 'xx' wipes away all the children and replaces them with xx.

Both cases are expected with the current specification. So in general, you are allowed to touch innerHTML in connectedCallback() unlike constructor() in which you aren't, but the readiness of children may differ.

I tested with Safari (10.1.1 and TP34), and all 4 cases show xx inside <tag-1>, while on Chrome only the third case show xx.

@rniwa @dominiccooney @domenic looks like the timings of running connectedCallback() microtask differ between Blink and WebKit. Is this an interop issue?

@tnikolai2
Copy link
Author

I do table tag, that contains column definition in nested tags. With fragment parser- it good, but with HTML parser need use mutation observers or some other hard logic to avoid this problems.

If custom component has nested components, it always must be parsed as fragment, otherwise what can do inside connectedCallback? If cannot see/modify nested elements - nested tags is almost useless

@dominiccooney
Copy link
Contributor

You might need a different callback, (imagine something like "parserPoppedEndTag"), to do what you want. Custom elements do not have that callback but the group could consider adding it.

I have heard from other authors that a callback at this time would be useful; the use case is to delay initialization work that would be invalidated by markup children (like OBJECT/PARAM.) You still have to use MutationObservers to track DOM changes; this use case is for performance optimization.

Depending on the timing of the callback it might also simplify when to start listening to mutation events (because today you get no clear signal whether your constructor was invoked for upgrade, and you should process existing children, or not. This callback could be an opportunity to put that code in one place.)

@annevk
Copy link
Collaborator

annevk commented Jul 12, 2017

That's the callback you'd need to implement the script element. However, the more such callbacks we add the slower parsing becomes.

@tnikolai2
Copy link
Author

Better in customElements.define add attribute "isContainer". This component must be connected only after parsing child content. It makes no sense to connect it before the childrens parsed, component anyway will change its content.

Also i have problem with removing children components for my componens like m-form,m-table,m-modaldiv. When i rewrite innerHTML in connectedCallback, for old removed children components anyway invoked connectedCallback.
I need use function __clear(el){
while (el.hasChildNodes()) {
__clear(el.lastChild)
el.removeChild(el.lastChild);
}
}

And in connectedCallback must do check firstly:
if(this.parentNode===undefined || this.parentNode==null ) {__clear(this); return;}

It not simple to get rid of removed components. For container component rewriting innerHTML must fully kill existing children components.

For container component before connectedCallback children must be only parsed. After container connectedCallback finished, need connect actual children content.

@annevk
Copy link
Collaborator

annevk commented Feb 17, 2018

@TakayoshiKochi FWIW, yes, that does seem like a problem. As far as I know Safari would be wrong there, but @domenic should know for sure. I thought we had tests for things like this?

@rniwa
Copy link
Collaborator

rniwa commented Apr 20, 2023

F2F resolution: closing in favor of #809

@rniwa rniwa closed this as completed Apr 20, 2023
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

5 participants