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

Restrict the types of nodes that could be assigned to a slot #351

Closed
rniwa opened this issue Nov 19, 2015 · 6 comments
Closed

Restrict the types of nodes that could be assigned to a slot #351

rniwa opened this issue Nov 19, 2015 · 6 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented Nov 19, 2015

It seems that we only want Text and Element nodes to be assigned to a slot. There is currently no mechanism to assign non-element nodes to a slot other than via the default slot so this is only relevant to default slots. Since only Text and Element nodes are visible, we can probably not even expose assignedSlot on non-Text CharacterData nodes.

@rniwa
Copy link
Collaborator Author

rniwa commented Nov 19, 2015

@hayatoito
Copy link
Contributor

I do not have a strong opinion on this. This sounds a minor issue and does not cause a real issue, IMO.

Do we really want to introduce this restriction? I'm fine that non-Text CharacterData is assigned to a slot, which is harmless.

If we want to introduce the restriction, that's easy, I think. We have to:

  1. Change the IDL

From:

partial interface NonDocumentTypeChildNode {
    readonly        attribute HTMLSlotElement? assignedSlot;
};

To:

partial interface Element {
    readonly        attribute HTMLSlotElement? assignedSlot;
};
partial interface TextNode {
    readonly        attribute HTMLSlotElement? assignedSlot;
};
  1. Tweak the slotting algorithm in the spec. Additional one sentence is enough.
  2. UA has to introduce "if () { } else {}" in implementing slotting algorithm . :)

@rniwa
Copy link
Collaborator Author

rniwa commented Jan 13, 2016

This actually reduces the amount of code needed in our engine (WebKit) so it's highly desirable for us.

@hayatoito
Copy link
Contributor

Okay. Then, it is worth doing. I'm okay to this minor change.

Out of curiosity, could you have a chance to share your experience briefly? Why does this change impact WebKit? I've not encountered such an issue in Blink. I'm afraid that I might have missed something.

@rniwa
Copy link
Collaborator Author

rniwa commented Jan 14, 2016

It's something to do with the way our style resolution works, and that has changed quite a bit between Blink and WebKit since Blink fork so I'm not certain how relevant our issue is.

@hayatoito
Copy link
Contributor

I got it. Thanks.

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

2 participants