Bug 1422747: Make 2-level nested insertion points work correctly. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 15 Dec 2017 00:03:42 +0100
changeset 711881 b0512c68e15708d5085578c54c82dcd515d403ec
parent 711880 ef8dd8a7e685924438ba18025ae81c5d26bc4e6b
child 743917 fdd877e1bfbcccb41a2720979dbc259ef1e6ea75
push id93189
push userbmo:emilio@crisal.io
push dateThu, 14 Dec 2017 23:14:19 +0000
bugs1422747
milestone59.0a1
Bug 1422747: Make 2-level nested insertion points work correctly. MozReview-Commit-ID: DGs6HCC4FYN
dom/xbl/nsBindingManager.cpp
dom/xbl/nsBindingManager.h
layout/reftests/bugs/reftest.list
--- 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