Skip to content

Fix and clarify more around CDATASection #301

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

Closed
wants to merge 2 commits into from
Closed

Fix and clarify more around CDATASection #301

wants to merge 2 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Aug 17, 2016

  • Make appendChild(cdata) and similar not throw.
  • Let nodeName be #cdata-section.
  • Include CDATASection as a node in an exhaustive enumeration.
  • Collapse cases of Text+PI+Comment to CharacterData where possible
    so that it's more clear that subtypes are included.
  • Say "depending on" instead of "switched" on.

Part of #300.

@foolip
Copy link
Member Author

foolip commented Aug 17, 2016

The normalize() stuff from #300 is not addressed here.

@@ -1488,7 +1488,7 @@ can be used to explore this matter in more detail.

<h3 id=node-trees>Node tree</h3>

<p>{{Document}}, {{DocumentType}}, {{DocumentFragment}}, {{Element}}, {{Text}},
<p>{{Document}}, {{DocumentType}}, {{DocumentFragment}}, {{Element}}, {{Text}}, {{CDATASection}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason CDATASection is not mentioned here is because ShadowRoot isn't either. We cannot have it both ways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so I suppose that this is then enumerating the least-derived interfaces that can be instantiated? Otherwise it'd collapse into just saying "Node" :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess it's "So the new policy is to list concrete classes (DocumentFragment) unless they’re a subclass of a concrete class (ShadowRoot) and not list abstract classes (CharacterData)."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do something else (e.g., also use abstract classes). But without whatwg/webidl#97 I'm not sure this can be improved much.

@annevk
Copy link
Member

annevk commented Aug 17, 2016

The only thing that seems like a real issue here is "<code>#cdata-section</code>".

@foolip
Copy link
Member Author

foolip commented Aug 17, 2016

I've split out nodeName into #302 but think some of what's left would be worthwhile, will rebase and comment inline.

This is already used in a number of places that "switch" on the
interface type.

Part of #330.
@foolip
Copy link
Member Author

foolip commented Aug 17, 2016

OK, so after looking at what remained again, I'm left with just eb9252f

Feel free to merge or reject and move on :)

@annevk
Copy link
Member

annevk commented Aug 17, 2016

I prefer the term "switch" from "switch case".

@annevk annevk closed this Aug 17, 2016
@foolip
Copy link
Member Author

foolip commented Aug 17, 2016

I don't understand, "switch case" doesn't appear in the spec. Do you prefer "depending on the context object" or "switching on the context object"? Both are now used and I picked the one that I thought was less likely to be blindly translated to switch-case in C++.

@annevk
Copy link
Member

annevk commented Aug 18, 2016

I prefer the latter, but perhaps you're right that the current setup doesn't work. In particular branching on type seems wrong.

@foolip
Copy link
Member Author

foolip commented Aug 22, 2016

New issue, or do you have a plan?

@annevk
Copy link
Member

annevk commented Aug 22, 2016

Branding in IDL is still the only thing that resolves this I think. New issue to track that on DOM side seems fine.

@foolip
Copy link
Member Author

foolip commented Aug 22, 2016

I see some discussion of branding in whatwg/webidl#125, is there a larger tracking issue for whatever is involved?

@annevk
Copy link
Member

annevk commented Aug 22, 2016

No, that's it. I think we could clarify DOM meanwhile by putting CDATASection checks before Text. Since Text checks are true for both, but CDATASection will only be true for CDATASection.

@annevk
Copy link
Member

annevk commented Aug 22, 2016

(Are you okay with deleting this branch btw?)

@foolip foolip deleted the more-cdata branch August 23, 2016 09:18
@foolip
Copy link
Member Author

foolip commented Aug 23, 2016

Branch deleted, all that's needed to reproduce is to change either "depending on the context object" or "switching on the context object" to the other for consistency.

@foolip
Copy link
Member Author

foolip commented Aug 23, 2016

One relevant difference between CDATASection and ShadowRoot is that CDATASection has its own nodeType. So switching on nodeType actually works for ShadowRoot, but not for CDATASection. Since we'll presumably never add a new nodeType, actually spelling out whether CDATASection is included anywhere that Text is mentioned would remove the risk of confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants