Closed Bug 1453828 Opened 3 years ago Closed 3 years ago

nsINode::IsContainerNode is confused

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mccr8, Assigned: bzbarsky)

Details

Attachments

(1 file)

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?
MozReview-Commit-ID: BC1iBI38wfJ
Attachment #8969868 - Flags: review?(continuation)
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.
Attachment #8969868 - Flags: review?(continuation)
Not a problem.  Thank you for not just stamping this!
Attachment #8969868 - Flags: review?(bugs)
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 bzbarsky@mozilla.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.
https://hg.mozilla.org/mozilla-central/rev/26e4719fcec6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.