Closed Bug 1453828 Opened 7 years ago Closed 7 years ago

nsINode::IsContainerNode is confused

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mccr8, Assigned: bzbarsky)

Details

Attachments

(1 file)

Fix IsContainerNode to not claim true for doctype nodes
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
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: 7 years ago
status-firefox61: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: