Closed
Bug 1096635
Opened 10 years ago
Closed 10 years ago
GetFlattenedTreeParent() returns an active <children> element in some XBL situations
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: MatsPalmgren_bugz, Assigned: wchen)
References
Details
Attachments
(5 files, 3 obsolete files)
1.19 KB,
application/vnd.mozilla.xul+xml
|
Details | |
2.02 KB,
patch
|
Details | Diff | Splinter Review | |
1.51 KB,
patch
|
Details | Diff | Splinter Review | |
1.25 KB,
application/vnd.mozilla.xul+xml
|
Details | |
5.36 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 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•10 years ago
|
||
Try this patch instead for demonstration of the bug.
Reporter | ||
Comment 4•10 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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
It does fix the previous testcase, but I still see the debug patch printout using this testcase with your patch.
Assignee | ||
Comment 9•10 years ago
|
||
Here is a more complete fix for updating the default content insertion parent.
Attachment #8522034 -
Attachment is obsolete: true
Reporter | ||
Comment 10•10 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
Assignee | ||
Comment 11•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=96b44ae8b690
Attachment #8522438 -
Attachment is obsolete: true
Attachment #8522550 -
Flags: review?(mrbkap)
Updated•10 years ago
|
Attachment #8522550 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75e7054ecc4
Assignee: mats → wchen
Assignee | ||
Updated•10 years ago
|
Attachment #8521592 -
Flags: review?(wchen)
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e75e7054ecc4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•