Closed Bug 1096635 Opened 8 years ago Closed 8 years ago

GetFlattenedTreeParent() returns an active <children> element in some XBL situations


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

Not set





(Reporter: MatsPalmgren_bugz, Assigned: wchen)




(5 files, 3 obsolete files)

Attached file Testcase
Bug 907396 is using GetFlattenedTreeParent() in more situations and I've
found what I think is a bug in some XBL cases.  I don't think it should
ever return an element that is IsActiveChildrenElement() but it does.
Attached patch fix (obsolete) — Splinter Review
This is a Try run on vanilla trunk (without bug 907396):
Attachment #8520259 - Flags: review?(wchen)
Comment on attachment 8520259 [details] [diff] [review]

Review of attachment 8520259 [details] [diff] [review]:

This patch doesn't look right to me. You are right that GetFlattenedTreeParent() shouldn't be returning an active <children> element. I think the correct way to fix this is to find out why the correct node isn't being set for the XBL insertion parent and fix it so that GetXBLInsertionParent() returns the right thing. I debugged the attached test case and I don't actually see where GetFlattenedTreeParent() is returning an active <children>, starting from #x, I get the following flattened tree parent chain: #x, #i, #l, <window> and this is what I would expect. Which node in the test case is exhibiting the buggy behavior?
Attachment #8520259 - Flags: review?(wchen) → review-
Attached patch debug patchSplinter Review
Try this patch instead for demonstration of the bug.
Perhaps we could land my patch as a wallpaper until we can find a better fix?
(I don't want to block landing bug 907396 (display:contents) on this for very long
because it's a pain to maintain.)
Flags: needinfo?(wchen)
I'll also look into this issue today. I don't want this to block bug 907396 either, so if we can't figure it out today I think that a wallpaper fix is OK because the more I dig into this the more it looks like a deeper XBL bug relating to how we install/uninstall bindings when nodes are removed/added from the document. However, I don't think that the uploaded patch is correct even as a wallpaper fix because the binding parent wouldn't necessarily be the flattened tree parent, this happens to correct for the test case because the parent of the <xbl:children> is the <xbl:content> element, if the parent were anything else, the flattened tree parent would be that element. I think a more correct wallpaper fix would be to return parent of the <xbl:children> unless it happens to be <xbl:content> then return the binding parent.
Flags: needinfo?(wchen)
Attached patch wallpaperSplinter Review
OK, that makes sense.  I've added a test ("display-contents-xbl-5.xul" in bug 907396)
that fails with the old patch.
Attachment #8520259 - Attachment is obsolete: true
Attachment #8521592 - Flags: review?(wchen)
When a <xbl:children> doesn't have inserted children, it uses the default content (children of the <xbl:children>). Right now, XBL dosen't handle updating the insertion parent of default content very well when nodes are dynamically updated, we only set it when anonymous content is intially generated. In the test case, we are appending a node to a <xbl:children> element that does not have inserted children, which means the default content will be used but the insertion parent is not updated.

mats: Let me know if this patch works to solve the problem. If so I'd like to make the default content behaviour a bit more correct (I've at least found one other related bug while investigating this) then get it landed.
Attached file Testcase #2
It does fix the previous testcase, but I still see the debug patch printout
using this testcase with your patch.
Here is a more complete fix for updating the default content insertion parent.
Attachment #8522034 - Attachment is obsolete: true
It works fine locally in my display:contents tree (without my wallpaper).
I pushed the whole thing to Try:
Attachment #8522550 - Flags: review?(mrbkap) → review+
Attachment #8521592 - Flags: review?(wchen)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.