Bug 1452889: Handle appending multiple items to a listbox correctly. r?bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 10 Apr 2018 11:15:39 +0200
changeset 779601 6b3f9e118949e22ccf37cd3bc79b9857d11b4a22
parent 779573 1cccef72c1ace0094e02afdfff42b400adf30e22
child 779602 25dd1edb6f97f4dec126f9c39c7131695d8a1ce6
push id105823
push userbmo:emilio@crisal.io
push dateTue, 10 Apr 2018 09:36:36 +0000
reviewersbz
bugs1452889, 1446368, 1303605, 1429088
milestone61.0a1
Bug 1452889: Handle appending multiple items to a listbox correctly. r?bz What happened in bug 1446368 is the following: We append two items to an empty listbox. We can't construct lazily because this is XUL, so that goes through IssueSingleInsertNotifications for each of both. When we insert the first one we call LazilyStyleNewChildRange _only on the first sibling_, yet the listbox code tries to construct frames for the next sibling too from CreateRows, yet the next sibling is unstyled, so we panic. Instead of handling it in ContentRangeInserted but not ContentAppended, just do it in the listbox-specific code instead, which looks slightly cleaner (though we can't assert we're constructing async). This should fix the case where the listbox is display: none or what not which, combined with the patch in bug 1303605, supersede the backed out patch in bug 1429088, which was backed out because listboxes suck. MozReview-Commit-ID: D7UQ41S6Ras
layout/base/nsCSSFrameConstructor.cpp
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -7107,29 +7107,28 @@ nsCSSFrameConstructor::ContentAppended(n
     // construction case, except when we're already constructing frames, in
     // which case we shouldn't need to do anything else.
     if (aInsertionKind == InsertionKind::Async) {
       LazilyStyleNewChildRange(aFirstNewContent, nullptr);
     }
     return;
   }
 
-  if (aInsertionKind == InsertionKind::Async &&
-      MaybeConstructLazily(CONTENTAPPEND, aFirstNewContent)) {
-    LazilyStyleNewChildRange(aFirstNewContent, nullptr);
-    return;
-  }
-
-  // We couldn't construct lazily. Make Servo eagerly traverse the new content
-  // if needed (when aInsertionKind == InsertionKind::Sync, we know that the
-  // styles are up-to-date already).
   if (aInsertionKind == InsertionKind::Async) {
+    if (MaybeConstructLazily(CONTENTAPPEND, aFirstNewContent)) {
+      LazilyStyleNewChildRange(aFirstNewContent, nullptr);
+      return;
+    }
+    // We couldn't construct lazily. Make Servo eagerly traverse the new content
+    // if needed (when aInsertionKind == InsertionKind::Sync, we know that the
+    // styles are up-to-date already).
     StyleNewChildRange(aFirstNewContent, nullptr);
   }
 
+
   LAYOUT_PHASE_TEMP_EXIT();
   if (MaybeRecreateForFrameset(parentFrame, aFirstNewContent, nullptr)) {
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
   LAYOUT_PHASE_TEMP_REENTER();
 
   if (parentFrame->IsLeaf()) {
@@ -7457,36 +7456,26 @@ nsCSSFrameConstructor::ContentRangeInser
     // XXX the GetContent() != child check is needed due to bug 135040.
     // Remove it once that's fixed.
     NS_ASSERTION(!child->GetPrimaryFrame() ||
                  child->GetPrimaryFrame()->GetContent() != child,
                  "asked to construct a frame for a node that already has a frame");
   }
 #endif
 
-  auto styleNewChildRangeEagerly =
-    [this, aInsertionKind, aStartChild, aEndChild]() {
-      // When aInsertionKind == InsertionKind::Sync, we know that the
-      // styles are up-to-date already.
-      if (aInsertionKind == InsertionKind::Async) {
-        StyleNewChildRange(aStartChild, aEndChild);
-      }
-    };
 
   bool isSingleInsert = (aStartChild->GetNextSibling() == aEndChild);
   NS_ASSERTION(isSingleInsert ||
                aInsertionKind == InsertionKind::Sync,
                "range insert shouldn't be lazy");
   NS_ASSERTION(isSingleInsert || aEndChild,
                "range should not include all nodes after aStartChild");
 
 #ifdef MOZ_XUL
   if (aStartChild->GetParent() && IsXULListBox(aStartChild->GetParent())) {
-    // For XUL list box, we need to style the new children eagerly.
-    styleNewChildRangeEagerly();
     if (isSingleInsert) {
       // The insert case in NotifyListBoxBody doesn't use "old next sibling".
       if (NotifyListBoxBody(mPresShell->GetPresContext(),
                             aStartChild, nullptr, nullptr, CONTENT_INSERTED)) {
         return;
       }
     } else {
       // We don't handle a range insert to a listbox parent, issue single
@@ -7562,25 +7551,26 @@ nsCSSFrameConstructor::ContentRangeInser
     // construction case, except when we're already constructing frames, in
     // which case we shouldn't need to do anything else.
     if (aInsertionKind == InsertionKind::Async) {
       LazilyStyleNewChildRange(aStartChild, aEndChild);
     }
     return;
   }
 
-  if (aInsertionKind == InsertionKind::Async &&
-      MaybeConstructLazily(CONTENTINSERT, aStartChild)) {
-    LazilyStyleNewChildRange(aStartChild, aEndChild);
-    return;
-  }
-
-  // We couldn't construct lazily. Make Servo eagerly traverse the new content
-  // if needed.
-  styleNewChildRangeEagerly();
+  if (aInsertionKind == InsertionKind::Async) {
+    if (MaybeConstructLazily(CONTENTINSERT, aStartChild)) {
+      LazilyStyleNewChildRange(aStartChild, aEndChild);
+      return;
+    }
+    // We couldn't construct lazily. Make Servo eagerly traverse the new content
+    // if needed (when aInsertionKind == InsertionKind::Sync, we know that the
+    // styles are up-to-date already).
+    StyleNewChildRange(aStartChild, aEndChild);
+  }
 
   bool isAppend, isRangeInsertSafe;
   nsIFrame* prevSibling = GetInsertionPrevSibling(&insertion, aStartChild,
                                                   &isAppend, &isRangeInsertSafe);
 
   // check if range insert is safe
   if (!isSingleInsert && !isRangeInsertSafe) {
     // must fall back to a single ContertInserted for each child in the range
@@ -11153,16 +11143,20 @@ nsCSSFrameConstructor::CreateListBoxCont
   if (aParentFrame) {
     nsFrameItems            frameItems;
     nsFrameConstructorState state(mPresShell,
                                   GetAbsoluteContainingBlock(aParentFrame, FIXED_POS),
                                   GetAbsoluteContainingBlock(aParentFrame, ABS_POS),
                                   GetFloatContainingBlock(aParentFrame),
                                   do_AddRef(mTempFrameTreeState));
 
+    if (aChild->IsElement() && !aChild->AsElement()->HasServoData()) {
+      mPresShell->StyleSet()->StyleNewSubtree(aChild->AsElement());
+    }
+
     RefPtr<ComputedStyle> computedStyle = ResolveComputedStyle(aChild);
 
     // Pre-check for display "none" - only if we find that, do we create
     // any frame at all
     const nsStyleDisplay* display = computedStyle->StyleDisplay();
 
     if (StyleDisplay::None == display->mDisplay) {
       *aNewFrame = nullptr;