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

Shadow DOM and comment nodes #411

Closed
kjax opened this issue Mar 2, 2016 · 19 comments
Closed

Shadow DOM and comment nodes #411

kjax opened this issue Mar 2, 2016 · 19 comments

Comments

@kjax
Copy link

kjax commented Mar 2, 2016

Currently a node is called slot assignable if it is either Text or Element.
knockout.js uses comment nodes as "virtual elements"
Would it be possible to add comment node to the list of slot assignable?

@rniwa
Copy link
Collaborator

rniwa commented Mar 2, 2016

We just removed the support for assigning those nodes in the issue 351. I would not like to reverse it.

@hayatoito
Copy link
Contributor

Yeah, according to #351, WebKit has a technical constraint.
I do not have a strong opinion, however, if WebKit can resolve a technical constraint, I'm totally fine to make all nodes slots assignable. It's up to WebKit, as of now.

@rniwa, I'm assuming that a technical constraint still exists in WebKit, right?

@rniwa
Copy link
Collaborator

rniwa commented Mar 3, 2016

Yes, if there is a strong use case for assigning comment nodes, we could add the support though.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

What exactly is the constraint? It seems like if we do more than Element, we should do Element and everything CharacterData. E.g., there's a strong chance CDATASection makes a comeback, would be weird to support Text but not CDATASection.

@rniwa
Copy link
Collaborator

rniwa commented Mar 3, 2016

@annevk the constraint is that we don't currently have a mechanism to assign anything but Element and Text node at the moment, and adding the support for other types of Node is rather difficult.

But I would be okay with supporting it if there is a valid use case (i.e. end user / developer scenario) that requires those node types to be assigned to a slot.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

Well, if you still support CDATASection... that's semantically equivalent to Text. It would be very weird if CDATASection didn't end up in the flattened tree. And I guess the use for Comment / ProcessingInstruction is to hack around limitations of the DOM. Annotations that can be used for further processing as per OP.

@hayatoito
Copy link
Contributor

@kjax, could you elaborate the use case? If that's reasonable, let me reverse the change and make every node slots assignable. That will make things simple in terms of the spec, and make you happy.

@annevk
Copy link
Collaborator

annevk commented Mar 4, 2016

I don't think every node should have this. There's no reason to assign, e.g., a ShadowRoot or DocumentType object, is there?

@hayatoito
Copy link
Contributor

Yeah, they never be assigned to a slot.
I meant that I will relax a necessary condition - called slot assignable. Even if ShadowRoot or Document meets the relaxed necessary condition, they never be assigned to a slot, as per the slotting algorithm. That does not change.

@annevk
Copy link
Collaborator

annevk commented Mar 4, 2016

I meant that I don't think we should give every node an assignedSlot property. Just those that need it. But maybe I still misunderstood you.

@hayatoito
Copy link
Contributor

Ops. I understand. Regarding IDL, I am going to revert this commit: f986bf8. Thus, only NonDocumentTypeChildNode will have assignedSlot property.

@kjax
Copy link
Author

kjax commented Mar 7, 2016

After further analysis I determined that the knockout.js case that I was worried about does not need the comment nodes to be assigned to slots. While knockout does use comments as active nodes, all of the action occurs in the light DOM. There is no need for the comments to appear in the flattened tree.

However please confirm that CDATA is treated as text.

@hayatoito
Copy link
Contributor

@kjax Thanks. The most reasonable choice for us is to just revert this change: f986bf8.

I'm assuming that WebKit would be okay, though it might be painful. I'm not 100% sure.

@rniwa
Copy link
Collaborator

rniwa commented Mar 8, 2016

I think what #411 (comment) says is that we still don't need to assign non-Text/CDATA/Element nodes to a slot. I think we would be fine with supporting CDATA.

@hayatoito
Copy link
Contributor

Yeah, supporting CDATA is the bottom line.

Do you still think that we have to restrict other nodes types, such as comment nodes?

@rniwa
Copy link
Collaborator

rniwa commented Mar 8, 2016

We would still prefer that. Since CDATASection is a subclass of Text, it's given that we support both Text and CDATASection nodes by the fact every CDATASection is a Text node.

@hayatoito
Copy link
Contributor

Then, we do not have to do anything, right?
At least, until WebKit can resolve a technical constraint, which is unlikely to happen soon, I am assuming.

@hayatoito
Copy link
Contributor

Let me close this issue without any action.

@rniwa
Copy link
Collaborator

rniwa commented Mar 8, 2016

Yup, thanks.

If someone comes up with a valid use case for assigning Comment and ProcessingInstruction to a default slot, then we can re-visit this.

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