Make XBL know about its insertion point, not insertion point parent.

RESOLVED FIXED in Firefox 58

Status

()

Core
DOM
RESOLVED FIXED
a month ago
20 days ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a month ago
That way we can know which insertion point to look up for distributed siblings.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8921059 [details]
Bug 1410895: Make XBL slots hold the insertion point, not the XBL parent.

https://reviewboard.mozilla.org/r/192026/#review197396

::: dom/xbl/nsBindingManager.cpp:222
(Diff revision 2)
>      }
>  
>      aContent->SetXBLBinding(nullptr, this);
>    }
>  
>    // Clear out insertion parent and content lists.

s/parent/point/

::: dom/xbl/nsXBLBinding.cpp:804
(Diff revision 2)
> -      UpdateInsertionParent(mInsertionPoints[i], mBoundElement);
> -    }
> -
> -    // Now that our inserted children no longer think they're inserted
> -    // anywhere, make sure our internal state reflects that as well.
>      ClearInsertionPoints();

It looks like when this code was added (<https://hg.mozilla.org/mozilla-central/rev/731a5fcf13289e02bbd86f06950a3b93c7357e12>) calling ClearInsertionPoints() just cleared the child arrays on the insertion points and did not update the insertion point on the elements inside those insertion points.  Which is why this code had to do the updating.

It also looks like <https://hg.mozilla.org/mozilla-central/rev/a98e26325e28> changed the behavior to have XBLChildrenElement::ClearInsertedChildren clear out the insertion parent unconditionally on the kids.  It did NOT use the same logic as UpdateInsertionParent, though: it just always set to null instead of sometimes setting to the bound element itself.  After this, the UpdateInsertionParent calls are in fact redundant...
Attachment #8921059 - Flags: review?(bzbarsky) → review+
Blake, do you know what the story was with the changes I mention in comment 4?  Why do we not need to preserve the "correct" insertion parent?
Flags: needinfo?(mrbkap)
Comment on attachment 8921066 [details]
Bug 1410895: Multiple cleanups on top.

https://reviewboard.mozilla.org/r/192036/#review198174
Attachment #8921066 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

28 days ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1ea2429943c2
Make XBL slots hold the insertion point, not the XBL parent. r=bz
https://hg.mozilla.org/integration/autoland/rev/89e379338c15
Multiple cleanups on top. r=bz

Comment 10

27 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ea2429943c2
https://hg.mozilla.org/mozilla-central/rev/89e379338c15
Status: NEW → RESOLVED
Last Resolved: 27 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I can't remember why that code was there. I've done some spelunking in the history but nothing jumped out at me. Sorry.
Flags: needinfo?(mrbkap)
You need to log in before you can comment on or make changes to this bug.