Bug 1591947 - Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 09 Nov 2019 16:48:04 +0000
changeset 501414 74ae5427c21f848b01a2e72fd81f339ba2a5b7a3
parent 501413 8f0a0c1d6868be403c601cecee01ef9e26a23bb4
child 501415 a1901e1e614fc9773275cbc683cb77ea53ea1c7a
push id36791
push usercsabou@mozilla.com
push dateSun, 10 Nov 2019 09:53:30 +0000
treeherdermozilla-central@72c52c0101cf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersTYLin
bugs1591947
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1591947 - Fix style changes from list-style-image to -moz-appearance for XUL images. r=TYLin Consider the following case: <image style="list-style-image: url(foo.png)"></image> image.style.MozAppearance = "something" The early return was preventing us from clearing the image. This is an ancient bug, but it has started happening in the browser chrome because the lack of lazy frame construction for XUL elements makes us construct elements with an outdated style, which means in this case that they wouldn't have the -moz-appearance rule applied yet. Differential Revision: https://phabricator.services.mozilla.com/D52112
layout/reftests/xul/image-appearance-dynamic-ref.xul
layout/reftests/xul/image-appearance-dynamic.xul
layout/reftests/xul/reftest.list
layout/xul/nsImageBoxFrame.cpp
layout/xul/nsImageBoxFrame.h
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/image-appearance-dynamic-ref.xul
@@ -0,0 +1,16 @@
+<?xml version="1.0"?>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <style xmlns="http://www.w3.org/1999/xhtml"><![CDATA[
+    image {
+      list-style-image: url(colors-16x8.png);
+      -moz-appearance: button-arrow-up;
+    }
+  ]]></style>
+  <hbox>
+    <image/>
+  </hbox>
+</window>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/xul/image-appearance-dynamic.xul
@@ -0,0 +1,24 @@
+<?xml version="1.0"?>
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="reftest-wait">
+  <style xmlns="http://www.w3.org/1999/xhtml"><![CDATA[
+    image {
+      list-style-image: url(colors-16x8.png);
+    }
+    .tweak {
+      -moz-appearance: button-arrow-up;
+    }
+  ]]></style>
+  <hbox>
+    <image/>
+  </hbox>
+  <script>
+  window.addEventListener("load", function() {
+    document.querySelector("image").className = "tweak";
+    document.documentElement.className = "";
+  });
+  </script>
+</window>
--- a/layout/reftests/xul/reftest.list
+++ b/layout/reftests/xul/reftest.list
@@ -60,16 +60,18 @@ fuzzy-if(skiaContent,0-1,0-60) fuzzy-if(
 == chrome://reftest/content/xul/object-fit-scale-down-svg-005.xul object-fit-scale-down-svg-005-ref.html
 == chrome://reftest/content/xul/object-fit-scale-down-svg-006.xul object-fit-scale-down-svg-006-ref.html
 == chrome://reftest/content/xul/object-position-png-001.xul object-position-png-001-ref.html
 == chrome://reftest/content/xul/object-position-png-002.xul object-position-png-002-ref.html
 
 == chrome://reftest/content/xul/stack-sizing-1.xul chrome://reftest/content/xul/stack-sizing-1-ref.xul
 == chrome://reftest/content/xul/stack-sizing-2.xul chrome://reftest/content/xul/stack-sizing-2-ref.xul
 
+== chrome://reftest/content/xul/image-appearance-dynamic.xul chrome://reftest/content/xul/image-appearance-dynamic-ref.xul
+
 # Tests for rendering SVG images in a XUL <treecell>:
 # XXXdholbert: These are marked as "random" right now, since they might not
 # render the images they trying to test in time for the reftest snapshot, per
 # bug 1218954.
 skip == chrome://reftest/content/xul/treecell-image-svg-1a.xul chrome://reftest/content/xul/treecell-image-svg-1-ref.xul # bug 1218954
 skip == chrome://reftest/content/xul/treecell-image-svg-1b.xul chrome://reftest/content/xul/treecell-image-svg-1-ref.xul # bug 1218954
 
 == chrome://reftest/content/xul/treechildren-padding-percent-1.xul chrome://reftest/content/xul/treechildren-padding-percent-1-ref.xul
--- a/layout/xul/nsImageBoxFrame.cpp
+++ b/layout/xul/nsImageBoxFrame.cpp
@@ -251,30 +251,19 @@ void nsImageBoxFrame::UpdateImage() {
             getter_AddRefs(mImageRequest), contentPolicyType);
 
         if (NS_SUCCEEDED(rv) && mImageRequest) {
           nsLayoutUtils::RegisterImageRequestIfAnimated(
               presContext, mImageRequest, &mRequestRegistered);
         }
       }
     }
-  } else {
-    // Only get the list-style-image if we aren't being drawn
-    // by a native theme.
-    auto* display = StyleDisplay();
-    if (!(display->HasAppearance() && nsBox::gTheme &&
-          nsBox::gTheme->ThemeSupportsWidget(nullptr, this,
-                                             display->mAppearance))) {
-      // get the list-style-image
-      imgRequestProxy* styleRequest = StyleList()->GetListStyleImage();
-      if (styleRequest) {
-        styleRequest->SyncClone(mListener, mContent->GetComposedDoc(),
-                                getter_AddRefs(mImageRequest));
-      }
-    }
+  } else if (auto* styleRequest = GetRequestFromStyle()) {
+    styleRequest->SyncClone(mListener, mContent->GetComposedDoc(),
+                            getter_AddRefs(mImageRequest));
   }
 
   if (!mImageRequest) {
     // We have no image, so size to 0
     mIntrinsicSize.SizeTo(0, 0);
   } else {
     // We don't want discarding or decode-on-draw for xul images.
     mImageRequest->StartDecoding(imgIContainer::FLAG_ASYNC_NOTIFY);
@@ -597,49 +586,51 @@ nsRect nsDisplayXULImage::GetDestRect() 
 bool nsImageBoxFrame::CanOptimizeToImageLayer() {
   bool hasSubRect = !mUseSrcAttr && (mSubRect.width > 0 || mSubRect.height > 0);
   if (hasSubRect) {
     return false;
   }
   return true;
 }
 
-//
-// DidSetComputedStyle
-//
-// When the ComputedStyle changes, make sure that all of our image is up to
-// date.
-//
+imgRequestProxy* nsImageBoxFrame::GetRequestFromStyle() {
+  const nsStyleDisplay* disp = StyleDisplay();
+  if (disp->HasAppearance() && nsBox::gTheme &&
+      nsBox::gTheme->ThemeSupportsWidget(nullptr, this, disp->mAppearance)) {
+    return nullptr;
+  }
+
+  return StyleList()->GetListStyleImage();
+}
+
 /* virtual */
-void nsImageBoxFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
-  nsLeafBoxFrame::DidSetComputedStyle(aOldComputedStyle);
+void nsImageBoxFrame::DidSetComputedStyle(ComputedStyle* aOldStyle) {
+  nsLeafBoxFrame::DidSetComputedStyle(aOldStyle);
 
   // Fetch our subrect.
   const nsStyleList* myList = StyleList();
   mSubRect = myList->GetImageRegion();  // before |mSuppressStyleCheck| test!
 
   if (mUseSrcAttr || mSuppressStyleCheck)
     return;  // No more work required, since the image isn't specified by style.
 
-  // If we're using a native theme implementation, we shouldn't draw anything.
-  const nsStyleDisplay* disp = StyleDisplay();
-  if (disp->HasAppearance() && nsBox::gTheme &&
-      nsBox::gTheme->ThemeSupportsWidget(nullptr, this, disp->mAppearance))
-    return;
-
-  // If list-style-image changes, we have a new image.
+  // If the image to use changes, we have a new image.
   nsCOMPtr<nsIURI> oldURI, newURI;
-  if (mImageRequest) mImageRequest->GetURI(getter_AddRefs(oldURI));
-  if (myList->GetListStyleImage())
-    myList->GetListStyleImage()->GetURI(getter_AddRefs(newURI));
+  if (mImageRequest) {
+    mImageRequest->GetURI(getter_AddRefs(oldURI));
+  }
+  if (auto* newImage = GetRequestFromStyle()) {
+    newImage->GetURI(getter_AddRefs(newURI));
+  }
   bool equal;
   if (newURI == oldURI ||  // handles null==null
       (newURI && oldURI && NS_SUCCEEDED(newURI->Equals(oldURI, &equal)) &&
-       equal))
+       equal)) {
     return;
+  }
 
   UpdateImage();
 }  // DidSetComputedStyle
 
 void nsImageBoxFrame::GetImageSize() {
   if (mIntrinsicSize.width > 0 && mIntrinsicSize.height > 0) {
     mImageSize.width = mIntrinsicSize.width;
     mImageSize.height = mIntrinsicSize.height;
--- a/layout/xul/nsImageBoxFrame.h
+++ b/layout/xul/nsImageBoxFrame.h
@@ -58,26 +58,33 @@ class nsImageBoxFrame final : public nsL
                                        ComputedStyle* aStyle);
 
   virtual void Init(nsIContent* aContent, nsContainerFrame* aParent,
                     nsIFrame* asPrevInFlow) override;
 
   virtual nsresult AttributeChanged(int32_t aNameSpaceID, nsAtom* aAttribute,
                                     int32_t aModType) override;
 
-  virtual void DidSetComputedStyle(ComputedStyle* aOldComputedStyle) override;
+  virtual void DidSetComputedStyle(ComputedStyle* aOldStyle) override;
 
   virtual void DestroyFrom(nsIFrame* aDestructRoot,
                            PostDestroyData& aPostDestroyData) override;
 
 #ifdef DEBUG_FRAME_DUMP
   virtual nsresult GetFrameName(nsAString& aResult) const override;
 #endif
 
   /**
+   * Gets the image request to be loaded from the current style.
+   *
+   * May be null if themed.
+   */
+  imgRequestProxy* GetRequestFromStyle();
+
+  /**
    * Update mUseSrcAttr from appropriate content attributes or from
    * style, throw away the current image, and load the appropriate
    * image.
    * */
   void UpdateImage();
 
   /**
    * Update mLoadFlags from content attributes. Does not attempt to reload the