Bug 1422747: Make 2-level nested insertion points work correctly.
MozReview-Commit-ID: DGs6HCC4FYN
--- a/dom/xbl/nsBindingManager.cpp
+++ b/dom/xbl/nsBindingManager.cpp
@@ -814,36 +814,80 @@ void
nsBindingManager::AppendAllSheets(nsTArray<StyleSheet*>& aArray)
{
EnumerateBoundContentBindings([&aArray](nsXBLBinding* aBinding) {
aBinding->PrototypeBinding()->AppendStyleSheetsTo(aArray);
return true;
});
}
-static void
-InsertAppendedContent(XBLChildrenElement* aPoint,
- nsIContent* aFirstNewContent)
+static uint32_t
+FindInsertionIndex(XBLChildrenElement* aPoint, nsIContent* aContent)
{
- int32_t insertionIndex;
- if (nsIContent* prevSibling = aFirstNewContent->GetPreviousSibling()) {
+ // First try to find the previous / next siblings.
+ for (nsIContent* prevSibling = aContent->GetPreviousSibling();
+ prevSibling;
+ prevSibling = prevSibling->GetPreviousSibling()) {
// If we have a previous sibling, then it must already be in aPoint. Find
// it and insert after it.
- insertionIndex = aPoint->IndexOfInsertedChild(prevSibling);
- MOZ_ASSERT(insertionIndex != -1);
+ int32_t insertionIndex = aPoint->IndexOfInsertedChild(prevSibling);
+ if (insertionIndex != -1) {
+ // Our insertion index is one after our previous sibling's index.
+ return insertionIndex + 1;
+ }
+ }
+
+ for (nsIContent* nextSibling = aContent->GetNextSibling();
+ nextSibling;
+ nextSibling = nextSibling->GetNextSibling()) {
+ int32_t insertionIndex = aPoint->IndexOfInsertedChild(nextSibling);
+ if (insertionIndex != -1) {
+ return insertionIndex;
+ }
+ }
- // Our insertion index is one after our previous sibling's index.
- ++insertionIndex;
- } else {
- // Otherwise, we append.
- // TODO This is wrong for nested insertion points. In that case, we need to
- // keep track of the right index to insert into.
- insertionIndex = aPoint->InsertedChildrenLength();
+ // Handle nested insertion points.
+ if (auto* ip = aContent->GetXBLInsertionPoint()) {
+ for (nsIContent* sibling = ip->GetPreviousSibling();
+ sibling;
+ sibling = sibling->GetPreviousSibling()) {
+ // FIXME(emilio): sibling could be yet another insertion point I think,
+ // and we'd get the wrong answer in this case, since they're not inserted
+ // into aPoint, so this should probably use the last inserted kid in that
+ // case, and recurse, and that stuff...
+ if (sibling->GetXBLInsertionPoint() == aPoint) {
+ int32_t insertionIndex = aPoint->IndexOfInsertedChild(sibling);
+ if (insertionIndex != -1) {
+ return insertionIndex + 1;
+ }
+ }
+ }
+
+ for (nsIContent* sibling = ip->GetNextSibling();
+ sibling;
+ sibling = sibling->GetNextSibling()) {
+ if (sibling->GetXBLInsertionPoint() == aPoint) {
+ int32_t insertionIndex = aPoint->IndexOfInsertedChild(sibling);
+ if (insertionIndex != -1) {
+ return insertionIndex;
+ }
+ }
+ }
}
+ // Otherwise just append.
+ return aPoint->InsertedChildrenLength();
+}
+
+
+static void
+InsertAppendedContent(XBLChildrenElement* aPoint, nsIContent* aFirstNewContent)
+{
+ uint32_t insertionIndex = FindInsertionIndex(aPoint, aFirstNewContent);
+
// Do the inserting.
for (nsIContent* currentChild = aFirstNewContent;
currentChild;
currentChild = currentChild->GetNextSibling()) {
aPoint->InsertInsertedChildAt(currentChild, insertionIndex++);
}
}
@@ -878,17 +922,17 @@ nsBindingManager::ContentAppended(nsIDoc
if (binding->HasFilteredInsertionPoints()) {
// There are filtered insertion points involved, handle each child
// separately.
// We could optimize this in the case when we've nested a few levels
// deep already, without hitting bindings that have filtered insertion
// points.
for (nsIContent* currentChild = aFirstNewContent; currentChild;
currentChild = currentChild->GetNextSibling()) {
- HandleChildInsertion(aContainer, currentChild, true);
+ HandleChildInsertion(aContainer, currentChild);
}
return;
}
point = binding->GetDefaultInsertionPoint();
if (!point) {
break;
@@ -915,17 +959,17 @@ nsBindingManager::ContentAppended(nsIDoc
} while (parent);
}
void
nsBindingManager::ContentInserted(nsIDocument* aDocument,
nsIContent* aContainer,
nsIContent* aChild)
{
- HandleChildInsertion(aContainer, aChild, false);
+ HandleChildInsertion(aContainer, aChild);
}
void
nsBindingManager::ContentRemoved(nsIDocument* aDocument,
nsIContent* aContainer,
nsIContent* aChild,
nsIContent* aPreviousSibling)
{
@@ -1032,18 +1076,17 @@ nsBindingManager::Traverse(nsIContent *a
cb.NoteXPCOMChild(aContent);
NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "[via binding manager] mWrapperTable value");
cb.NoteXPCOMChild(value);
}
}
void
nsBindingManager::HandleChildInsertion(nsIContent* aContainer,
- nsIContent* aChild,
- bool aAppend)
+ nsIContent* aChild)
{
MOZ_ASSERT(aChild, "Must have child");
XBLChildrenElement* point = nullptr;
nsIContent* parent = aContainer;
// Handle insertion of default content.
if (parent && parent->IsActiveChildrenElement()) {
@@ -1064,33 +1107,17 @@ nsBindingManager::HandleChildInsertion(n
}
point = binding->FindInsertionPointFor(aChild);
if (!point) {
break;
}
// Insert the child into the proper insertion point.
- // TODO If there were multiple insertion points, this approximation can be
- // wrong. We need to re-run the distribution algorithm. In the meantime,
- // this should work well enough.
- uint32_t index = aAppend ? point->InsertedChildrenLength() : 0;
- for (nsIContent* currentSibling = aChild->GetPreviousSibling();
- currentSibling;
- currentSibling = currentSibling->GetPreviousSibling()) {
- // If we find one of our previous siblings in the insertion point, the
- // index following it is the correct insertion point. Otherwise, we guess
- // based on whether we're appending or inserting.
- int32_t pointIndex = point->IndexOfInsertedChild(currentSibling);
- if (pointIndex != -1) {
- index = pointIndex + 1;
- break;
- }
- }
-
+ uint32_t index = FindInsertionIndex(point, aChild);
point->InsertInsertedChildAt(aChild, index);
nsIContent* newParent = point->GetParent();
if (newParent == parent) {
break;
}
parent = newParent;
--- a/dom/xbl/nsBindingManager.h
+++ b/dom/xbl/nsBindingManager.h
@@ -179,19 +179,17 @@ public:
bool AnyBindingHasDocumentStateDependency(mozilla::EventStates aStateMask);
protected:
nsIXPConnectWrappedJS* GetWrappedJS(nsIContent* aContent);
nsresult SetWrappedJS(nsIContent* aContent, nsIXPConnectWrappedJS* aResult);
// Called by ContentAppended and ContentInserted to handle a single child
// insertion. aChild must not be null. aContainer may be null.
- // aAppend is true if this child is being appended, not inserted.
- void HandleChildInsertion(nsIContent* aContainer, nsIContent* aChild,
- bool aAppend);
+ void HandleChildInsertion(nsIContent* aContainer, nsIContent* aChild);
// Same as ProcessAttachedQueue, but also nulls out
// mProcessAttachedQueueEvent
void DoProcessAttachedQueue();
// Post an event to process the attached queue.
void PostProcessAttachedQueueEvent();
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1244,18 +1244,18 @@ fuzzy-if(webrender,16-16,3425-3425) == 4
== 467084-2.html 467084-2-ref.html
== 467444-1.html 467444-1-ref.html
== 467460-1.html 467460-1-ref.html
== 468473-1.xul 468473-1-ref.xul
== 468546-1.xhtml 468546-1-ref.xhtml
== 471356-1.html 471356-1-ref.html
== 471594-1.xhtml 471594-1-ref.html
fuzzy(255,15) == 472020-1a.xul 472020-1-ref.xul
-fails == 472020-1b.xul 472020-1-ref.xul
-fails == 472020-2.xul 472020-2-ref.xul
+== 472020-1b.xul 472020-1-ref.xul
+== 472020-2.xul 472020-2-ref.xul
== 472500-1.xul 472500-1-ref.xul
== 472769-1a.html 472769-1-ref.html
== 472769-1b.html 472769-1-ref.html
== 472769-2.html 472769-2-ref.html
== 472769-3.html 472769-3-ref.html
== 473847-1.xul 473847-1-ref.xul
fuzzy-if(skiaContent,1,16) == 474336-1.xul 474336-1-ref.xul
== 474417-1.html 474417-1-ref.html