Bug 1149357: Properly update responsive images for density changes. r?dholbert draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 23 May 2018 11:19:49 +0200
changeset 799746 c8e825f5aeff5ef12ca99fa5b407f0fd85100556
parent 799745 9ee17decddbf1dc441daa089a5c6333e5174f974
child 799747 9f462222fd130b5c9b557aa48590cce612914fe0
push id111161
push userbmo:emilio@crisal.io
push dateFri, 25 May 2018 10:25:09 +0000
reviewersdholbert
bugs1149357
milestone62.0a1
Bug 1149357: Properly update responsive images for density changes. r?dholbert Before that we were not notifying the image frame in any way if we ended up not doing a load, and we were instead relying on the reflow the viewport resize caused to get the new density in ComputeSize from the content node (but nowhere else, since that's the bug part 1 fixes). This was generally unsound, since you can stash random media in a sizes= attribute, which don't necessarily needs to cause a reflow. Now we need to notify necessarily because nsImageFrame stores the adjusted intrinsic size. mCurrentDensity could also get out of sync as well, when the selected image density changed, but we ended up returning early because our source hadn't change in the first early exit. This patch moves us to a model where we don't re-trigger loads for density changes if the source doesn't change (unless we pass aAlwaysLoad when we need to, per spec). This matches our previous behavior (without the bugginess of not updating the intrinsic size), and also Chromium, at least. This changes behavior in one case, which is when we don't load the same source node, but we have the same source URL, and the density does change. This could happen with <picture> and two <source>s with same source and different media and sizes. This makes our behavior consistent with the behavior we have when both the source and the density doesn't change. Blink and WebKit do trigger a second image load both when the source changes without changing density and when density changes. I'll file a spec issue, since per: https://html.spec.whatwg.org/#reacting-to-environment-changes We should be triggering the load when the density changes but the source doesn't as well, but no UA does that. I filed https://github.com/whatwg/html/issues/3709 with a little summary of the situation and what I think the behavior should be (which is what this patch implements). That being said, I'll update the impl if the spec people think otherwise :). MozReview-Commit-ID: Eqy16ygHRLo
dom/html/HTMLImageElement.cpp
dom/html/HTMLImageElement.h
layout/generic/nsImageFrame.cpp
layout/generic/nsImageFrame.h
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001-ref.html
testing/web-platform/tests/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001.html
--- a/dom/html/HTMLImageElement.cpp
+++ b/dom/html/HTMLImageElement.cpp
@@ -8,16 +8,17 @@
 #include "mozilla/dom/HTMLImageElementBinding.h"
 #include "nsGkAtoms.h"
 #include "nsStyleConsts.h"
 #include "nsPresContext.h"
 #include "nsMappedAttributes.h"
 #include "nsSize.h"
 #include "nsDocument.h"
 #include "nsIDocument.h"
+#include "nsImageFrame.h"
 #include "nsIScriptContext.h"
 #include "nsIURL.h"
 #include "nsIIOService.h"
 #include "nsIServiceManager.h"
 #include "nsContentUtils.h"
 #include "nsContainerFrame.h"
 #include "nsNodeInfoManager.h"
 #include "mozilla/MouseEvents.h"
@@ -944,68 +945,93 @@ HTMLImageElement::InResponsiveMode()
   // will happen from the microtask, and we should behave responsively in the
   // interim
   return mResponsiveSelector ||
          mPendingImageLoadTask ||
          HaveSrcsetOrInPicture();
 }
 
 bool
-HTMLImageElement::SelectedSourceMatchesLast(nsIURI* aSelectedSource, double aSelectedDensity)
+HTMLImageElement::SelectedSourceMatchesLast(nsIURI* aSelectedSource)
 {
   // If there was no selected source previously, we don't want to short-circuit the load.
   // Similarly for if there is no newly selected source.
   if (!mLastSelectedSource || !aSelectedSource) {
     return false;
   }
   bool equal = false;
-  return NS_SUCCEEDED(mLastSelectedSource->Equals(aSelectedSource, &equal)) && equal &&
-      aSelectedDensity == mCurrentDensity;
+  return NS_SUCCEEDED(mLastSelectedSource->Equals(aSelectedSource, &equal)) && equal;
 }
 
 nsresult
 HTMLImageElement::LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad)
 {
-  nsresult rv = NS_ERROR_FAILURE;
+  double currentDensity = 1.0; // default to 1.0 for the src attribute case
+
+  // Helper to update state when only density may have changed (i.e., the source
+  // to load hasn't changed, and we don't do any request at all). We need (apart
+  // from updating our internal state) to tell the image frame because its
+  // intrinsic size may have changed.
+  //
+  // In the case we actually trigger a new load, that load will trigger a call
+  // to nsImageFrame::NotifyNewCurrentRequest, which takes care of that for us.
+  auto UpdateDensityOnly = [&]() -> void {
+    if (mCurrentDensity == currentDensity) {
+      return;
+    }
+    mCurrentDensity = currentDensity;
+    if (nsImageFrame* f = do_QueryFrame(GetPrimaryFrame())) {
+      f->ResponsiveContentDensityChanged();
+    }
+  };
 
   if (aForce) {
-    // In responsive mode we generally want to re-run the full
-    // selection algorithm whenever starting a new load, per
-    // spec. This also causes us to re-resolve the URI as appropriate.
-    if (!UpdateResponsiveSource() && !aAlwaysLoad) {
+    // In responsive mode we generally want to re-run the full selection
+    // algorithm whenever starting a new load, per spec.
+    //
+    // This also causes us to re-resolve the URI as appropriate.
+    const bool sourceChanged = UpdateResponsiveSource();
+
+    if (mResponsiveSelector) {
+      currentDensity = mResponsiveSelector->GetSelectedImageDensity();
+    }
+
+    if (!sourceChanged && !aAlwaysLoad) {
+      UpdateDensityOnly();
       return NS_OK;
     }
+  } else if (mResponsiveSelector) {
+    currentDensity = mResponsiveSelector->GetSelectedImageDensity();
   }
 
+  nsresult rv = NS_ERROR_FAILURE;
   nsCOMPtr<nsIURI> selectedSource;
-  double currentDensity = 1.0; // default to 1.0 for the src attribute case
   if (mResponsiveSelector) {
     nsCOMPtr<nsIURI> url = mResponsiveSelector->GetSelectedImageURL();
     nsCOMPtr<nsIPrincipal> triggeringPrincipal = mResponsiveSelector->GetSelectedImageTriggeringPrincipal();
     selectedSource = url;
-    currentDensity = mResponsiveSelector->GetSelectedImageDensity();
-    if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource, currentDensity)) {
+    if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource)) {
+      UpdateDensityOnly();
       return NS_OK;
     }
     if (url) {
       rv = LoadImage(url, aForce, aNotify, eImageLoadType_Imageset,
                      triggeringPrincipal);
     }
   } else {
     nsAutoString src;
     if (!GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
       CancelImageRequests(aNotify);
       rv = NS_OK;
     } else {
-      nsIDocument* doc = GetOurOwnerDoc();
-      if (doc) {
-        StringToURI(src, doc, getter_AddRefs(selectedSource));
-        if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource, currentDensity)) {
-          return NS_OK;
-        }
+      nsIDocument* doc = OwnerDoc();
+      StringToURI(src, doc, getter_AddRefs(selectedSource));
+      if (!aAlwaysLoad && SelectedSourceMatchesLast(selectedSource)) {
+        UpdateDensityOnly();
+        return NS_OK;
       }
 
       // If we have a srcset attribute or are in a <picture> element,
       // we always use the Imageset load type, even if we parsed no
       // valid responsive sources from either, per spec.
       rv = LoadImage(src, aForce, aNotify,
                      HaveSrcsetOrInPicture() ? eImageLoadType_Imageset
                                              : eImageLoadType_Normal,
--- a/dom/html/HTMLImageElement.h
+++ b/dom/html/HTMLImageElement.h
@@ -325,18 +325,18 @@ protected:
   // True if we have a srcset attribute or a <picture> parent, regardless of if
   // any valid responsive sources were parsed from either.
   bool HaveSrcsetOrInPicture();
 
   // True if we are using the newer image loading algorithm. This will be the
   // only mode after Bug 1076583
   bool InResponsiveMode();
 
-  // True if the given URL and density equal the last URL and density that was loaded by this element.
-  bool SelectedSourceMatchesLast(nsIURI* aSelectedSource, double aSelectedDensity);
+  // True if the given URL equals the last URL that was loaded by this element.
+  bool SelectedSourceMatchesLast(nsIURI* aSelectedSource);
 
   // Resolve and load the current mResponsiveSelector (responsive mode) or src
   // attr image.
   nsresult LoadSelectedImage(bool aForce, bool aNotify, bool aAlwaysLoad);
 
   // True if this string represents a type we would support on <source type>
   static bool SupportedPictureSourceType(const nsAString& aType);
 
--- a/layout/generic/nsImageFrame.cpp
+++ b/layout/generic/nsImageFrame.cpp
@@ -609,22 +609,18 @@ nsImageFrame::OnSizeAvailable(imgIReques
   }
 
   MarkNeedsDisplayItemRebuild();
 
   if (intrinsicSizeChanged && (mState & IMAGE_GOTINITIALREFLOW)) {
     // Now we need to reflow if we have an unconstrained size and have
     // already gotten the initial reflow
     if (!(mState & IMAGE_SIZECONSTRAINED)) {
-      nsIPresShell *presShell = presContext->GetPresShell();
-      NS_ASSERTION(presShell, "No PresShell.");
-      if (presShell) {
-        presShell->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
+      PresShell()->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
                                     NS_FRAME_IS_DIRTY);
-      }
     } else {
       // We've already gotten the initial reflow, and our size hasn't changed,
       // so we're ready to request a decode.
       MaybeDecodeForPredictedSize();
     }
 
     mPrevImage = nullptr;
   }
@@ -705,16 +701,35 @@ nsImageFrame::OnLoadComplete(imgIRequest
     return NS_ERROR_FAILURE;
   }
 
   NotifyNewCurrentRequest(aRequest, aStatus);
   return NS_OK;
 }
 
 void
+nsImageFrame::ResponsiveContentDensityChanged()
+{
+  if (!(mState & IMAGE_GOTINITIALREFLOW)) {
+    return;
+  }
+
+  if (!mImage) {
+    return;
+  }
+
+  if (!UpdateIntrinsicSize(mImage) && !UpdateIntrinsicRatio(mImage)) {
+    return;
+  }
+
+  PresShell()->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
+                                NS_FRAME_IS_DIRTY);
+}
+
+void
 nsImageFrame::NotifyNewCurrentRequest(imgIRequest *aRequest,
                                       nsresult aStatus)
 {
   nsCOMPtr<imgIContainer> image;
   aRequest->GetImage(getter_AddRefs(image));
   NS_ASSERTION(image || NS_FAILED(aStatus), "Successful load with no container?");
 
   // May have to switch sizes here!
@@ -733,21 +748,18 @@ nsImageFrame::NotifyNewCurrentRequest(im
     mIntrinsicSize.width.SetCoordValue(0);
     mIntrinsicSize.height.SetCoordValue(0);
     mIntrinsicRatio.SizeTo(0, 0);
   }
 
   if (mState & IMAGE_GOTINITIALREFLOW) { // do nothing if we haven't gotten the initial reflow yet
     if (intrinsicSizeChanged) {
       if (!(mState & IMAGE_SIZECONSTRAINED)) {
-        nsIPresShell *presShell = PresContext()->GetPresShell();
-        if (presShell) {
-          presShell->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
+        PresShell()->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
                                       NS_FRAME_IS_DIRTY);
-        }
       } else {
         // We've already gotten the initial reflow, and our size hasn't changed,
         // so we're ready to request a decode.
         MaybeDecodeForPredictedSize();
       }
 
       mPrevImage = nullptr;
     }
--- a/layout/generic/nsImageFrame.h
+++ b/layout/generic/nsImageFrame.h
@@ -100,16 +100,18 @@ public:
                              nsIFrame::Cursor& aCursor) override;
   virtual nsresult AttributeChanged(int32_t aNameSpaceID,
                                     nsAtom* aAttribute,
                                     int32_t aModType) override;
 
   void OnVisibilityChange(Visibility aNewVisibility,
                           const Maybe<OnNonvisible>& aNonvisibleAction = Nothing()) override;
 
+  void ResponsiveContentDensityChanged();
+
 #ifdef ACCESSIBILITY
   virtual mozilla::a11y::AccType AccessibleType() override;
 #endif
 
   virtual bool IsFrameOfType(uint32_t aFlags) const override
   {
     return nsAtomicContainerFrame::IsFrameOfType(aFlags &
       ~(nsIFrame::eReplaced | nsIFrame::eReplacedSizing));
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -182180,16 +182180,28 @@
       [
        "/html/semantics/embedded-content/the-img-element/document-base-url-ref.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001.html": [
+    [
+     "/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001.html",
+     [
+      [
+       "/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "html/semantics/embedded-content/the-video-element/video_content_image.htm": [
     [
      "/html/semantics/embedded-content/the-video-element/video_content_image.htm",
      [
       [
        "/html/semantics/embedded-content/the-video-element/video_content-ref.htm",
        "=="
       ]
@@ -286184,16 +286196,21 @@
      {}
     ]
    ],
    "html/semantics/embedded-content/the-img-element/resources/cat.jpg": [
     [
      {}
     ]
    ],
+   "html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "html/semantics/embedded-content/the-img-element/sizes/sizes-iframed.sub.html": [
     [
      {}
     ]
    ],
    "html/semantics/embedded-content/the-img-element/srcset/common.js": [
     [
      {}
@@ -577664,16 +577681,24 @@
   "html/semantics/embedded-content/the-img-element/resources/cat.jpg": [
    "d8bdb2208a32d2200afb173368c38826fede8476",
    "support"
   ],
   "html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html": [
    "22040d8543a29c1e4f1708017096a5f9de478549",
    "testharness"
   ],
+  "html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001-ref.html": [
+   "d2a087e2d55f7d0a2de3a0008c149932151a69f0",
+   "support"
+  ],
+  "html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001.html": [
+   "0256768b16722bf4a8e6e1fb41b48e321509610f",
+   "reftest"
+  ],
   "html/semantics/embedded-content/the-img-element/sizes/sizes-iframed.sub.html": [
    "47e56d828d8c366a95d0ea77571a1dbcaaca3164",
    "support"
   ],
   "html/semantics/embedded-content/the-img-element/srcset/common.js": [
    "1928faeb6e177e80b56d049ff81439512538fc36",
    "support"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001-ref.html
@@ -0,0 +1,4 @@
+<!doctype html>
+<title>Test reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<iframe width="500" height="500" srcdoc='<!doctype html><img alt="FAIL" srcset="/images/green-256x256.png 100w" style="max-width: 100%" sizes="10px">'></iframe>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/sizes/sizes-dynamic-001.html
@@ -0,0 +1,7 @@
+<!doctype html>
+<title>Image intrinsic size specified via sizes attribute reacts properly to media changes</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="match" href="sizes-dynamic-001-ref.html">
+<link rel="help" href="https://html.spec.whatwg.org/#sizes-attributes">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1149357">
+<iframe onload="this.width = '500'" width="200" height="500" srcdoc='<!doctype html><img srcset="/images/green-256x256.png 100w" style="max-width: 100%" sizes="(min-width: 400px) 10px, 20px">'></iframe>