Bug 1787072 - Avoid useless reframes setting the src attribute of a broken image that already has an image frame. r=dholbert

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

layout/base/RestyleManager.cpp

@@ -486,9 +486,15 @@ static bool StateChangeMayAffectFrame(const Element& aElement,
   }
 
   if (aElement.IsHTMLElement(nsGkAtoms::img)) {
-    // Loading state doesn't affect <img>, see
-    // `nsImageFrame::ShouldCreateImageFrameFor`.
-    return brokenChanged;
+    if (!brokenChanged) {
+      // Loading state doesn't affect <img>, see
+      // `nsImageFrame::ImageFrameTypeForElement`.
+      return false;
+    }
+    const bool needsImageFrame =
+        nsImageFrame::ImageFrameTypeFor(aElement, *aFrame.Style()) !=
+        nsImageFrame::ImageFrameType::None;
+    return needsImageFrame != aFrame.IsImageFrameOrSubclass();
   }
 
   if (aElement.IsSVGElement(nsGkAtoms::image)) {
@@ -2705,7 +2711,7 @@ bool RestyleManager::ProcessPostTraversal(Element* aElement,
   // We should really fix the weird primary frame mapping for image maps
   // (bug 135040)...
   if (styleFrame && styleFrame->GetContent() != aElement) {
-    MOZ_ASSERT(static_cast<nsImageFrame*>(do_QueryFrame(styleFrame)));
+    MOZ_ASSERT(styleFrame->IsImageFrameOrSubclass());
     styleFrame = nullptr;
   }
 

layout/base/nsCSSFrameConstructor.cpp

@@ -3515,7 +3515,9 @@ nsCSSFrameConstructor::FindGeneratedImageData(const Element& aElement,
 const nsCSSFrameConstructor::FrameConstructionData*
 nsCSSFrameConstructor::FindImgData(const Element& aElement,
                                    ComputedStyle& aStyle) {
-  if (!nsImageFrame::ShouldCreateImageFrameFor(aElement, aStyle)) {
+  if (nsImageFrame::ImageFrameTypeFor(aElement, aStyle) !=
+      nsImageFrame::ImageFrameType::ForElementRequest) {
+    // content: url gets handled by the generic code-path.
     return nullptr;
   }
 
@@ -3527,7 +3529,8 @@ nsCSSFrameConstructor::FindImgData(const Element& aElement,
 const nsCSSFrameConstructor::FrameConstructionData*
 nsCSSFrameConstructor::FindImgControlData(const Element& aElement,
                                           ComputedStyle& aStyle) {
-  if (!nsImageFrame::ShouldCreateImageFrameFor(aElement, aStyle)) {
+  if (nsImageFrame::ImageFrameTypeFor(aElement, aStyle) !=
+      nsImageFrame::ImageFrameType::ForElementRequest) {
     return nullptr;
   }
 
@@ -5356,7 +5359,8 @@ nsCSSFrameConstructor::FindElementData(const Element& aElement,
 
   // Check for 'content: <image-url>' on the element (which makes us ignore
   // 'display' values other than 'none' or 'contents').
-  if (nsImageFrame::ShouldCreateImageFrameForContent(aElement, aStyle)) {
+  if (nsImageFrame::ShouldCreateImageFrameForContentProperty(aElement,
+                                                             aStyle)) {
     static constexpr FrameConstructionData sImgData(
         NS_NewImageFrameForContentProperty);
     return &sImgData;

layout/generic/nsIFrame.cpp

@@ -7852,6 +7852,11 @@ bool nsIFrame::IsBlockFrameOrSubclass() const {
   return !!thisAsBlock;
 }
 
+bool nsIFrame::IsImageFrameOrSubclass() const {
+  const nsImageFrame* asImage = do_QueryFrame(this);
+  return !!asImage;
+}
+
 static nsIFrame* GetNearestBlockContainer(nsIFrame* frame) {
   // The block wrappers we use to wrap blocks inside inlines aren't
   // described in the CSS spec.  We need to make them not be containing

layout/generic/nsIFrame.h

@@ -3325,6 +3325,12 @@ class nsIFrame : public nsQueryFrame {
    */
   bool IsBlockFrameOrSubclass() const;
 
+  /**
+   * Returns true if the frame is an instance of nsImageFrame or one of its
+   * subclasses.
+   */
+  bool IsImageFrameOrSubclass() const;
+
   /**
    * Returns true if the frame is an instance of SVGGeometryFrame or one
    * of its subclasses.

layout/generic/nsImageFrame.cpp

@@ -188,8 +188,8 @@ bool nsDisplayGradient::CreateWebRenderCommands(
 StaticRefPtr<nsImageFrame::IconLoad> nsImageFrame::gIconLoad;
 
 // test if the width and height are fixed, looking at the style data
-// This is used by nsImageFrame::ShouldCreateImageFrameFor and should
-// not be used for layout decisions.
+// This is used by nsImageFrame::ImageFrameTypeFor and should not be used for
+// layout decisions.
 static bool HaveSpecifiedSize(const nsStylePosition* aStylePosition) {
   // check the width and height values in the reflow input's style struct
   // - if width and height are specified as either coord or percentage, then
@@ -834,7 +834,7 @@ static bool HasAltText(const Element& aElement) {
   return aElement.HasNonEmptyAttr(nsGkAtoms::alt);
 }
 
-bool nsImageFrame::ShouldCreateImageFrameForContent(
+bool nsImageFrame::ShouldCreateImageFrameForContentProperty(
     const Element& aElement, const ComputedStyle& aStyle) {
   if (aElement.IsRootOfNativeAnonymousSubtree()) {
     return false;
@@ -848,39 +848,42 @@ bool nsImageFrame::ShouldCreateImageFrameForContent(
 }
 
 // Check if we want to use an image frame or just let the frame constructor make
-// us into an inline.
+// us into an inline, and if so, which kind of image frame should we create.
 /* static */
-bool nsImageFrame::ShouldCreateImageFrameFor(const Element& aElement,
-                                             const ComputedStyle& aStyle) {
-  if (ShouldCreateImageFrameForContent(aElement, aStyle)) {
+auto nsImageFrame::ImageFrameTypeFor(const Element& aElement,
+                                     const ComputedStyle& aStyle)
+    -> ImageFrameType {
+  if (ShouldCreateImageFrameForContentProperty(aElement, aStyle)) {
     // Prefer the content property, for compat reasons, see bug 1484928.
-    return false;
+    return ImageFrameType::ForContentProperty;
   }
 
   if (ImageOk(aElement.State())) {
     // Image is fine or loading; do the image frame thing
-    return true;
+    return ImageFrameType::ForElementRequest;
   }
 
   if (aStyle.StyleUIReset()->mMozForceBrokenImageIcon) {
-    return true;
+    return ImageFrameType::ForElementRequest;
   }
 
   // if our "do not show placeholders" pref is set, skip the icon
   if (gIconLoad && gIconLoad->mPrefForceInlineAltText) {
-    return false;
+    return ImageFrameType::None;
   }
 
   if (!HasAltText(aElement)) {
-    return true;
+    return ImageFrameType::ForElementRequest;
   }
 
-  if (aElement.OwnerDoc()->GetCompatibilityMode() == eCompatibility_NavQuirks) {
-    // FIXME(emilio): We definitely don't reframe when this changes...
-    return HaveSpecifiedSize(aStyle.StylePosition());
+  // FIXME(emilio, bug 1788767): We definitely don't reframe when
+  // HaveSpecifiedSize changes...
+  if (aElement.OwnerDoc()->GetCompatibilityMode() == eCompatibility_NavQuirks &&
+      HaveSpecifiedSize(aStyle.StylePosition())) {
+    return ImageFrameType::ForElementRequest;
   }
 
-  return false;
+  return ImageFrameType::None;
 }
 
 void nsImageFrame::Notify(imgIRequest* aRequest, int32_t aType,
@@ -936,7 +939,6 @@ void nsImageFrame::OnSizeAvailable(imgIRequest* aRequest,
 }
 
 void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) {
-  MOZ_ASSERT(aRequest);
   if (SizeIsAvailable(aRequest)) {
     // This is valid and for the current request, so update our stored image
     // container, orienting according to our style.
@@ -954,19 +956,8 @@ void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) {
       return;
     }
   }
-  bool intrinsicSizeOrRatioChanged = [&] {
-    // NOTE(emilio): We intentionally want to call both functions and avoid
-    // short-circuiting.
-    bool intrinsicSizeChanged = UpdateIntrinsicSize();
-    bool intrinsicRatioChanged = UpdateIntrinsicRatio();
-    return intrinsicSizeChanged || intrinsicRatioChanged;
-  }();
 
-  if (intrinsicSizeOrRatioChanged) {
-    // Our aspect-ratio property value changed, and an embedding <object> or
-    // <embed> might care about that.
-    MaybeSendIntrinsicSizeAndRatioToEmbedder();
-  }
+  UpdateIntrinsicSizeAndRatio();
 
   if (!GotInitialReflow()) {
     return;
@@ -974,19 +965,6 @@ void nsImageFrame::UpdateImage(imgIRequest* aRequest, imgIContainer* aImage) {
 
   // We're going to need to repaint now either way.
   InvalidateFrame();
-
-  if (intrinsicSizeOrRatioChanged) {
-    // Now we need to reflow if we have an unconstrained size and have
-    // already gotten the initial reflow.
-    if (!(mState & IMAGE_SIZECONSTRAINED)) {
-      PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange,
-                                    NS_FRAME_IS_DIRTY);
-    } else if (PresShell()->IsActive()) {
-      // We've already gotten the initial reflow, and our size hasn't changed,
-      // so we're ready to request a decode.
-      MaybeDecodeForPredictedSize();
-    }
-  }
 }
 
 void nsImageFrame::OnFrameUpdate(imgIRequest* aRequest,
@@ -1085,16 +1063,32 @@ void nsImageFrame::OnLoadComplete(imgIRequest* aRequest, nsresult aStatus) {
   NotifyNewCurrentRequest(aRequest, aStatus);
 }
 
+void nsImageFrame::ElementStateChanged(ElementState aStates) {
+  if (!(aStates & ElementState::BROKEN)) {
+    return;
+  }
+  if (mKind != Kind::ImageElement) {
+    return;
+  }
+  if (!ImageOk(mContent->AsElement()->State())) {
+    UpdateImage(nullptr, nullptr);
+  }
+}
+
 void nsImageFrame::ResponsiveContentDensityChanged() {
-  if (!GotInitialReflow()) {
-    return;
-  }
+  UpdateIntrinsicSizeAndRatio();
+}
 
-  if (!mImage) {
-    return;
-  }
+void nsImageFrame::UpdateIntrinsicSizeAndRatio() {
+  bool intrinsicSizeOrRatioChanged = [&] {
+    // NOTE(emilio): We intentionally want to call both functions and avoid
+    // short-circuiting.
+    bool intrinsicSizeChanged = UpdateIntrinsicSize();
+    bool intrinsicRatioChanged = UpdateIntrinsicRatio();
+    return intrinsicSizeChanged || intrinsicRatioChanged;
+  }();
 
-  if (!UpdateIntrinsicSize() && !UpdateIntrinsicRatio()) {
+  if (!intrinsicSizeOrRatioChanged) {
     return;
   }
 
@@ -1102,8 +1096,20 @@ void nsImageFrame::ResponsiveContentDensityChanged() {
   // <embed> might care about that.
   MaybeSendIntrinsicSizeAndRatioToEmbedder();
 
-  PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange,
-                                NS_FRAME_IS_DIRTY);
+  if (!GotInitialReflow()) {
+    return;
+  }
+
+  // Now we need to reflow if we have an unconstrained size and have
+  // already gotten the initial reflow.
+  if (!HasAnyStateBits(IMAGE_SIZECONSTRAINED)) {
+    PresShell()->FrameNeedsReflow(this, IntrinsicDirty::StyleChange,
+                                  NS_FRAME_IS_DIRTY);
+  } else if (PresShell()->IsActive()) {
+    // We've already gotten the initial reflow, and our size hasn't changed,
+    // so we're ready to request a decode.
+    MaybeDecodeForPredictedSize();
+  }
 }
 
 void nsImageFrame::NotifyNewCurrentRequest(imgIRequest* aRequest,

layout/generic/nsImageFrame.h

@@ -99,6 +99,7 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback {
       const Maybe<OnNonvisible>& aNonvisibleAction = Nothing()) final;
 
   void ResponsiveContentDensityChanged();
+  void ElementStateChanged(mozilla::dom::ElementState) override;
   void SetupForContentURLRequest();
   bool ShouldShowBrokenImageIcon() const;
 
@@ -140,16 +141,21 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback {
    * Returns whether we should replace an element with an image corresponding to
    * its 'content' CSS property.
    */
-  static bool ShouldCreateImageFrameForContent(const mozilla::dom::Element&,
-                                               const ComputedStyle&);
+  static bool ShouldCreateImageFrameForContentProperty(
+      const mozilla::dom::Element&, const ComputedStyle&);
 
   /**
    * Function to test whether given an element and its style, that element
-   * should get an image frame.  Note that this method is only used by the
-   * frame constructor; it's only here because it uses gIconLoad for now.
+   * should get an image frame, and if so, which kind of image frame (for
+   * `content`, or for the element itself).
    */
-  static bool ShouldCreateImageFrameFor(const mozilla::dom::Element&,
-                                        const ComputedStyle&);
+  enum class ImageFrameType {
+    ForContentProperty,
+    ForElementRequest,
+    None,
+  };
+  static ImageFrameType ImageFrameTypeFor(const mozilla::dom::Element&,
+                                          const ComputedStyle&);
 
   ImgDrawResult DisplayAltFeedback(gfxContext& aRenderingContext,
                                    const nsRect& aDirtyRect, nsPoint aPt,
@@ -218,6 +224,8 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback {
   void ReflowChildren(nsPresContext*, const ReflowInput&,
                       const mozilla::LogicalSize& aImageSize);
 
+  void UpdateIntrinsicSizeAndRatio();
+
  protected:
   nsImageFrame(ComputedStyle* aStyle, nsPresContext* aPresContext, ClassID aID)
       : nsImageFrame(aStyle, aPresContext, aID, Kind::ImageElement) {}
@@ -354,8 +362,8 @@ class nsImageFrame : public nsAtomicContainerFrame, public nsIReflowCallback {
   bool IsPendingLoad(imgIRequest*) const;
 
   /**
-   * Updates mImage based on the current image request (cannot be null), and the
-   * image passed in (can be null), and invalidate layout and paint as needed.
+   * Updates mImage based on the current image request, and the image passed in
+   * (both can be null), and invalidate layout and paint as needed.
    */
   void UpdateImage(imgIRequest*, imgIContainer*);
 

layout/style/res/html.css

@@ -647,8 +647,12 @@ hr[size="1"] {
   border-style: solid none none none;
 }
 
-input:-moz-broken::before,
-img:-moz-broken::before {
+/* Note that we only intend for the alt content to show up if the image is
+ * broken. But non-broken images/inputs will have a replaced box, and thus we
+ * won't we don't generate the pseudo-element anyways. This prevents
+ * unnecessary reframing when images become broken / non-broken. */
+input[type=image]::before,
+img::before {
   content: -moz-alt-content !important;
   unicode-bidi: isolate;
 }

layout/style/test/mochitest.ini

@@ -267,6 +267,7 @@ skip-if = verify || toolkit == 'android' # Bug 1455824
 [test_hover_on_part.html]
 [test_html_attribute_computed_values.html]
 [test_ident_escaping.html]
+[test_img_src_causing_reflow.html]
 [test_import_preload.html]
 support-files = slow_load.sjs
 # Test is slightly racy and on Android it fails frequently enough to be

layout/style/test/test_img_src_causing_reflow.html

@@ -0,0 +1,36 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Test for bug 1787072</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+img {
+  width: 100px;
+  height: 100px;
+  background-color: blue;
+}
+</style>
+<img> <!-- Initially broken -->
+<script>
+add_task(async function() {
+  const utils = SpecialPowers.DOMWindowUtils;
+  const img = document.querySelector("img");
+  img.getBoundingClientRect();
+
+  let origFramesConstructed = utils.framesConstructed;
+  let origFramesReflowed = utils.framesReflowed;
+
+  let error = new Promise(r => img.addEventListener("error", r, { once: true }));
+
+  // Doesn't need to be an actual image.
+  img.src = "/some-valid-url";
+
+  img.getBoundingClientRect();
+  is(origFramesReflowed, utils.framesReflowed, "Shouldn't have reflowed when going broken -> loading");
+  is(origFramesConstructed, utils.framesConstructed, "Shouldn't have reflowed when going broken -> loading");
+
+  await error;
+
+  is(origFramesReflowed, utils.framesReflowed, "Shouldn't have reflowed when going loading -> broken");
+  is(origFramesConstructed, utils.framesConstructed, "Shouldn't have reflowed when going loading -> broken");
+});
+</script>