Bug 1585882 - Fix the case where where a node with an up-to-date style loses its frame due to a DOM mutation of siblings. r=heycam

This fixes another edge-case that I thought of while debugging this, I think
this makes our behavior correct now. The comment and test-case should be
self-descriptive.

Differential Revision: https://phabricator.services.mozilla.com/D48135

--HG--
extra : moz-landing-system : lando

layout/base/RestyleManager.cpp

@@ -3211,10 +3211,6 @@ void RestyleManager::ProcessAllPendingAttributeAndStateInvalidations() {
   ClearSnapshots();
 }
 
-bool RestyleManager::HasPendingRestyleAncestor(Element* aElement) const {
-  return Servo_HasPendingRestyleAncestor(aElement);
-}
-
 void RestyleManager::UpdateOnlyAnimationStyles() {
   bool doCSS = PresContext()->EffectCompositor()->HasPendingStyleUpdates();
   if (!doCSS) {

layout/base/RestyleManager.h

@@ -375,8 +375,6 @@ class RestyleManager {
   // ::first-line.
   void ReparentComputedStyleForFirstLine(nsIFrame*);
 
-  bool HasPendingRestyleAncestor(dom::Element* aElement) const;
-
   /**
    * Performs a Servo animation-only traversal to compute style for all nodes
    * with the animation-only dirty bit in the document.

layout/style/nsComputedDOMStyle.cpp

@@ -100,7 +100,8 @@ already_AddRefed<CSSValue> GetBackgroundList(
 }
 
 // Whether aDocument needs to restyle for aElement
-static bool ElementNeedsRestyle(Element* aElement, nsAtom* aPseudo) {
+static bool ElementNeedsRestyle(Element* aElement, nsAtom* aPseudo,
+                                bool aMayNeedToFlushLayout) {
   const Document* doc = aElement->GetComposedDoc();
   if (!doc) {
     // If the element is out of the document we don't return styles for it, so
@@ -171,7 +172,7 @@ static bool ElementNeedsRestyle(Element* aElement, nsAtom* aPseudo) {
   // Note this is different from Gecko: we only check if any ancestor needs
   // to restyle _itself_, not descendants, since dirty descendants can be
   // another subtree.
-  return restyleManager->HasPendingRestyleAncestor(aElement);
+  return Servo_HasPendingRestyleAncestor(aElement, aMayNeedToFlushLayout);
 }
 
 /**
@@ -762,9 +763,61 @@ void nsComputedDOMStyle::SetFrameComputedStyle(mozilla::ComputedStyle* aStyle,
   mPresShellId = mPresShell->GetPresShellId();
 }
 
-bool nsComputedDOMStyle::NeedsToFlushStyle() const {
+static bool MayNeedToFlushLayout(nsCSSPropertyID aPropID) {
+  switch (aPropID) {
+    case eCSSProperty_width:
+    case eCSSProperty_height:
+    case eCSSProperty_block_size:
+    case eCSSProperty_inline_size:
+    case eCSSProperty_line_height:
+    case eCSSProperty_grid_template_rows:
+    case eCSSProperty_grid_template_columns:
+    case eCSSProperty_perspective_origin:
+    case eCSSProperty_transform_origin:
+    case eCSSProperty_transform:
+    case eCSSProperty_border_top_width:
+    case eCSSProperty_border_bottom_width:
+    case eCSSProperty_border_left_width:
+    case eCSSProperty_border_right_width:
+    case eCSSProperty_border_block_start_width:
+    case eCSSProperty_border_block_end_width:
+    case eCSSProperty_border_inline_start_width:
+    case eCSSProperty_border_inline_end_width:
+    case eCSSProperty_top:
+    case eCSSProperty_right:
+    case eCSSProperty_bottom:
+    case eCSSProperty_left:
+    case eCSSProperty_inset_block_start:
+    case eCSSProperty_inset_block_end:
+    case eCSSProperty_inset_inline_start:
+    case eCSSProperty_inset_inline_end:
+    case eCSSProperty_padding_top:
+    case eCSSProperty_padding_right:
+    case eCSSProperty_padding_bottom:
+    case eCSSProperty_padding_left:
+    case eCSSProperty_padding_block_start:
+    case eCSSProperty_padding_block_end:
+    case eCSSProperty_padding_inline_start:
+    case eCSSProperty_padding_inline_end:
+    case eCSSProperty_margin_top:
+    case eCSSProperty_margin_right:
+    case eCSSProperty_margin_bottom:
+    case eCSSProperty_margin_left:
+    case eCSSProperty_margin_block_start:
+    case eCSSProperty_margin_block_end:
+    case eCSSProperty_margin_inline_start:
+    case eCSSProperty_margin_inline_end:
+      return true;
+    default:
+      return false;
+  }
+}
+
+bool nsComputedDOMStyle::NeedsToFlushStyle(nsCSSPropertyID aPropID) const {
+  bool mayNeedToFlushLayout = MayNeedToFlushLayout(aPropID);
+
   // We always compute styles from the element's owner document.
-  if (ElementNeedsRestyle(mElement, mPseudo)) {
+  if (ElementNeedsRestyle(mElement, mPseudo, mayNeedToFlushLayout)) {
     return true;
   }
 
@@ -774,7 +827,7 @@ bool nsComputedDOMStyle::NeedsToFlushStyle() const {
   while (doc->StyleOrLayoutObservablyDependsOnParentDocumentLayout()) {
     Document* parentDocument = doc->GetInProcessParentDocument();
     Element* element = parentDocument->FindContentForSubDocument(doc);
-    if (ElementNeedsRestyle(element, nullptr)) {
+    if (ElementNeedsRestyle(element, nullptr, mayNeedToFlushLayout)) {
       return true;
     }
     doc = parentDocument;
@@ -837,15 +890,14 @@ static bool PaddingNeedsUsedValue(const LengthPercentage& aValue,
 
 bool nsComputedDOMStyle::NeedsToFlushLayout(nsCSSPropertyID aPropID) const {
   MOZ_ASSERT(aPropID != eCSSProperty_UNKNOWN);
+  if (aPropID == eCSSPropertyExtra_variable) {
+    return false;
+  }
   nsIFrame* outerFrame = GetOuterFrame();
   if (!outerFrame) {
     return false;
   }
   nsIFrame* frame = StyleFrame(outerFrame);
-  if (aPropID == eCSSPropertyExtra_variable) {
-    return false;
-  }
-
   auto* style = frame->Style();
   if (nsCSSProps::PropHasFlags(aPropID, CSSPropFlags::IsLogical)) {
     aPropID = Servo_ResolveLogicalProperty(aPropID, style);
@@ -963,7 +1015,7 @@ void nsComputedDOMStyle::UpdateCurrentStyleSources(nsCSSPropertyID aPropID) {
   }
 
   DebugOnly<bool> didFlush = false;
-  if (NeedsToFlushStyle()) {
+  if (NeedsToFlushStyle(aPropID)) {
     didFlush = true;
     // We look at the frame in NeedsToFlushLayout, so flush frames, not only
     // styles.
@@ -971,6 +1023,7 @@ void nsComputedDOMStyle::UpdateCurrentStyleSources(nsCSSPropertyID aPropID) {
   }
 
   if (NeedsToFlushLayout(aPropID)) {
+    MOZ_ASSERT(MayNeedToFlushLayout(aPropID));
     didFlush = true;
     Flush(*document, FlushType::Layout);
 #ifdef DEBUG

layout/style/nsComputedDOMStyle.h

@@ -327,7 +327,7 @@ class nsComputedDOMStyle final : public nsDOMCSSDeclaration,
 
   // Find out if we can safely skip flushing (i.e. pending restyles do not
   // affect our element).
-  bool NeedsToFlushStyle() const;
+  bool NeedsToFlushStyle(nsCSSPropertyID) const;
   // Find out if we need to flush layout of the document, depending on the
   // property that was requested.
   bool NeedsToFlushLayout(nsCSSPropertyID) const;

servo/ports/geckolib/glue.rs

@@ -6136,7 +6136,10 @@ pub extern "C" fn Servo_ProcessInvalidations(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_HasPendingRestyleAncestor(element: &RawGeckoElement) -> bool {
+pub extern "C" fn Servo_HasPendingRestyleAncestor(
+    element: &RawGeckoElement,
+    may_need_to_flush_layout: bool,
+) -> bool {
     let mut has_yet_to_be_styled = false;
     let mut element = Some(GeckoElement(element));
     while let Some(e) = element {
@@ -6162,6 +6165,19 @@ pub extern "C" fn Servo_HasPendingRestyleAncestor(element: &RawGeckoElement) ->
             if has_yet_to_be_styled && !data.styles.is_display_none() {
                 return true;
             }
+            // Ideally, DOM mutations wouldn't affect layout trees of siblings.
+            //
+            // In practice, this can happen because Gecko deals pretty badly
+            // with some kinds of content insertion and removals.
+            //
+            // If we may need to flush layout, we need frames to accurately
+            // determine whether we'll actually flush, so if we have to
+            // reconstruct we need to flush style, which is what will take care
+            // of ensuring that frames are constructed, even if the style itself
+            // is up-to-date.
+            if may_need_to_flush_layout && data.damage.contains(GeckoRestyleDamage::reconstruct()) {
+                return true;
+            }
         }
         has_yet_to_be_styled = data.is_none();
 

testing/web-platform/tests/css/cssom/getComputedStyle-layout-dependent-removed-ib-sibling.html

@@ -0,0 +1,30 @@
+<!doctype html>
+<title>getComputedStyle() returns the right style for layout-dependent properties for nodes that have had an IB sibling removed</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1585882">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<style>
+  div {
+    width: 100%;
+  }
+</style>
+<span>
+  <div></div>
+  <div></div>
+</span>
+<script>
+test(() => {
+  let first = document.querySelector("div");
+  let second = document.querySelector("div + div");
+
+  let oldWidth = getComputedStyle(second).width;
+  assert_true(oldWidth.indexOf("px") !== -1, "Should return the used value for width");
+
+  first.remove();
+
+  assert_equals(getComputedStyle(second).width, oldWidth, "Should return the used value for width (after sibling removal)");
+}, "getComputedStyle() should return the correct used value for nodes that have had an IB-split sibling removed");
+</script>