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

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mats, Assigned: wchen)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
Posted 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.
Reporter

Comment 1

5 years ago
Posted patch fix (obsolete) — Splinter Review
This is a Try run on vanilla trunk (without bug 907396):
https://tbpl.mozilla.org/?tree=Try&rev=4454286818e8
Attachment #8520259 - Flags: review?(wchen)
Assignee

Comment 2

5 years ago
Comment on attachment 8520259 [details] [diff] [review]
fix

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-
Reporter

Comment 3

5 years ago
Posted patch debug patchSplinter Review
Try this patch instead for demonstration of the bug.
Reporter

Comment 4

5 years ago
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)
Assignee

Comment 5

5 years ago
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)
Reporter

Comment 6

5 years ago
Posted 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)
Assignee

Comment 7

5 years ago
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.
Reporter

Comment 8

5 years ago
Posted file Testcase #2
It does fix the previous testcase, but I still see the debug patch printout
using this testcase with your patch.
Assignee

Comment 9

5 years ago
Here is a more complete fix for updating the default content insertion parent.
Attachment #8522034 - Attachment is obsolete: true
Reporter

Comment 10

5 years ago
It works fine locally in my display:contents tree (without my wallpaper).
I pushed the whole thing to Try:
https://tbpl.mozilla.org/?tree=Try&rev=872e9e1ebbfd
Attachment #8522550 - Flags: review?(mrbkap) → review+
Assignee

Updated

5 years ago
Attachment #8521592 - Flags: review?(wchen)
https://hg.mozilla.org/mozilla-central/rev/e75e7054ecc4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.