Closed Bug 1453828 Opened 3 years ago Closed 3 years ago
INode::Is Container Node is confused
See bug 1449670 comment 5: (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5) > Boy, this code is confused. eDATA_NODE is only true when the node is > comment, textnode, cdata, or PI, so that check is redundant. And this check > is making doctypes be treated as IsContainerNode().... > > Anyway, if we want to preserve the current sematics we can just do: > > return IsElement() || !IsCharacterData(); > > but I think returning true here for doctypes is wrong and we should fix this > to actually do something sane. Mind filing a followup on this?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I have no idea what a container node is. Is that some kind of spec concept I can read about?
The spec has https://dom.spec.whatwg.org/#interface-parentnode which is kind of similar, I think. But fundamentally, in terms of our code a ContainerNode is whatever consumers expect. But I think all the consumers expect it to be a node that can have children.
Comment on attachment 8969868 [details] [diff] [review] Fix IsContainerNode to not claim true for doctype nodes I tried reading some code and the spec link you posted, but I'm not confident enough that I have a clue about what is going on here. Sorry.
Not a problem. Thank you for not just stamping this!
Huh, what did I review in bug 1380367.
Comment on attachment 8969868 [details] [diff] [review] Fix IsContainerNode to not claim true for doctype nodes Attrs can't have children anymore, so you should be able to drop that.
Attachment #8969868 - Flags: review?(bugs) → review+
> Attrs can't have children anymore, so you should be able to drop that. Done.
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/26e4719fcec6 Fix IsContainerNode to not claim true for doctype nodes. r=mccr8
FWIW this landed with the wrong reviewer in the commit message. :)
Argh, yes, I just noticed that. Sorry. :(
Let me know if you want me to back out and reland....
No, it doesn't matter. I just thought I'd mention it here for posterity.
You need to log in before you can comment on or make changes to this bug.