Bug 1524966 - Apply CSS clip inside the transform rather than outside. r=mstange, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 04 Feb 2019 23:06:02 +0000
changeset 515816 617b5ae30c05521e63f774a7b6bc3083e5f866fe
parent 515815 e31b4c03e372ed023b78c8e9662d768c53bfb0aa
child 515817 bf7ac7d809cf4edbb96f56e75ace2fed59a3e41a
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange, RyanVM
bugs1524966
milestone66.0
Bug 1524966 - Apply CSS clip inside the transform rather than outside. r=mstange, a=RyanVM I think this is the less complex way to get the desired behavior here... The extra AutoSaveRestore is needed to handle the clipped + transformed + fixed case, I added a test for that too. Differential Revision: https://phabricator.services.mozilla.com/D18555
layout/generic/nsFrame.cpp
testing/web-platform/tests/css/css-masking/clip/clip-filter-order-ref.html
testing/web-platform/tests/css/css-masking/clip/clip-filter-order.html
testing/web-platform/tests/css/css-masking/clip/clip-transform-order-2.html
testing/web-platform/tests/css/css-masking/clip/clip-transform-order.html
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -2938,29 +2938,33 @@ void nsIFrame::BuildDisplayListForStacki
     eSeparatorTransforms,
     eOpacity,
     eFilter,
     eBlendContainer
   };
 
   nsDisplayListBuilder::AutoContainerASRTracker contASRTracker(aBuilder);
 
-  DisplayListClipState::AutoSaveRestore cssClip(aBuilder);
-  {
-    // The clip property clips everything, including filters and what not.
-    if (auto contentClip = GetClipPropClipRect(disp, effects, GetSize())) {
-      nsPoint offset = isTransformed
-                           ? GetOffsetToCrossDoc(
-                                 aBuilder->FindReferenceFrameFor(GetParent()))
-                           : aBuilder->GetCurrentFrameOffsetToReferenceFrame();
-
-      aBuilder->IntersectDirtyRect(*contentClip);
-      aBuilder->IntersectVisibleRect(*contentClip);
-      cssClip.ClipContentDescendants(*contentClip + offset);
-    }
+  auto cssClip = GetClipPropClipRect(disp, effects, GetSize());
+  auto ApplyClipProp = [&](DisplayListClipState::AutoSaveRestore& aClipState) {
+    if (!cssClip) {
+      return;
+    }
+    nsPoint offset = aBuilder->GetCurrentFrameOffsetToReferenceFrame();
+    aBuilder->IntersectDirtyRect(*cssClip);
+    aBuilder->IntersectVisibleRect(*cssClip);
+    aClipState.ClipContentDescendants(*cssClip + offset);
+  };
+
+  // The CSS clip property is effectively inside the transform, but outside the
+  // filters. So if we're not transformed we can apply it just here for
+  // simplicity, instead of on each of the places that handle clipCapturedBy.
+  DisplayListClipState::AutoSaveRestore untransformedCssClip(aBuilder);
+  if (!isTransformed) {
+    ApplyClipProp(untransformedCssClip);
   }
 
   // If there is a current clip, then depending on the container items we
   // create, different things can happen to it. Some container items simply
   // propagate the clip to their children and aren't clipped themselves.
   // But other container items, especially those that establish a different
   // geometry for their contents (e.g. transforms), capture the clip on
   // themselves and unset the clip for their contents. If we create more than
@@ -2989,16 +2993,30 @@ void nsIFrame::BuildDisplayListForStacki
     clipCapturedBy = ContainerItemType::eFilter;
   }
 
   DisplayListClipState::AutoSaveRestore clipState(aBuilder);
   if (clipCapturedBy != ContainerItemType::eNone) {
     clipState.Clear();
   }
 
+  DisplayListClipState::AutoSaveRestore transformedCssClip(aBuilder);
+  if (isTransformed) {
+    // FIXME(emilio, bug 1525159): In the case we have a both a transform _and_
+    // filters, this clips the input to the filters as well, which is not
+    // correct (clipping by the `clip` property is supposed to happen after
+    // applying the filter effects, per [1].
+    //
+    // This is not a regression though, since we used to do that anyway before
+    // bug 1514384, and even without the transform we get it wrong.
+    //
+    // [1]: https://drafts.fxtf.org/css-masking/#placement
+    ApplyClipProp(transformedCssClip);
+  }
+
   mozilla::UniquePtr<HitTestInfo> hitTestInfo;
 
   nsDisplayListCollection set(aBuilder);
   Maybe<nsRect> clipForMask;
   {
     DisplayListClipState::AutoSaveRestore nestedClipState(aBuilder);
     nsDisplayListBuilder::AutoInTransformSetter inTransformSetter(aBuilder,
                                                                   inTransform);
@@ -3279,16 +3297,17 @@ void nsIFrame::BuildDisplayListForStacki
     if (separator) {
       ct.TrackContainer(separator);
     }
 
     resultList.AppendToTop(&participants);
   }
 
   if (isTransformed) {
+    transformedCssClip.Restore();
     if (clipCapturedBy == ContainerItemType::eTransform) {
       // Restore clip state now so nsDisplayTransform is clipped properly.
       clipState.Restore();
     }
     // Revert to the dirtyrect coming in from the parent, without our transform
     // taken into account.
     aBuilder->SetVisibleRect(visibleRectOutsideTransform);
     // Revert to the outer reference frame and offset because all display
--- a/testing/web-platform/tests/css/css-masking/clip/clip-filter-order-ref.html
+++ b/testing/web-platform/tests/css/css-masking/clip/clip-filter-order-ref.html
@@ -1,21 +1,24 @@
 <!DOCTYPE html>
 <meta charset="utf-8">
 <title>Clip should be applied after filtering</title>
 <link rel="author" title="Philip Rogers" href="mailto:pdr@chromium.org">
 
+<style>
+  body { margin: 0 }
+  #testcase {
+    position: absolute;
+    left: 10px;
+    width: 200px;
+    height: 200px;
+    background: green;
+    will-change: transform;
+  }
+</style>
+
 <div>
   <p>Expected: A green box.<br>
   There should be no red visible.<br>
   There should be a crisp, clipped edge around the green box (no blurring).</p>
 </div>
 
-<style>
-  #testcase {
-    position: absolute;
-    width: 200px;
-    height: 200px;
-    background: green;
-    will-change: transform;
-  }
-</style>
 <div id="testcase"></div>
--- a/testing/web-platform/tests/css/css-masking/clip/clip-filter-order.html
+++ b/testing/web-platform/tests/css/css-masking/clip/clip-filter-order.html
@@ -1,25 +1,28 @@
 <!DOCTYPE html>
 <meta charset="utf-8">
 <title>Clip should be applied after filtering</title>
 <link rel="author" title="Philip Rogers" href="mailto:pdr@chromium.org">
 <link rel="help" href="https://drafts.fxtf.org/css-masking-1/#placement">
 <link rel="match" href="clip-filter-order-ref.html">
 
-<div>
-  <p>Expected: A green box.<br>
-  There should be no red visible.<br>
-  There should be a crisp, clipped edge around the green box (no blurring).</p>
-</div>
-
 <style>
+  body { margin: 0 }
   #testcase {
     position: absolute;
+    left: 10px;
     width: 400px;
     height: 400px;
     background: green;
     will-change: transform;
     filter: drop-shadow(16px 16px 20px red);
     clip: rect(0px, 200px, 200px, 0px);
   }
 </style>
+
+<div>
+  <p>Expected: A green box.<br>
+  There should be no red visible.<br>
+  There should be a crisp, clipped edge around the green box (no blurring).</p>
+</div>
+
 <div id="testcase"></div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-masking/clip/clip-transform-order-2.html
@@ -0,0 +1,30 @@
+<!doctype html>
+<title>Clips should be applied before transforms (when fixed positioned too)</title>
+<link rel="help" href="https://drafts.fxtf.org/css-masking-1/#placement">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1524966">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<link rel="match" href="clip-filter-order-ref.html">
+
+<style>
+  body { margin: 0; overflow: hidden; }
+  #testcase {
+    position: fixed;
+    left: -100px;
+    width: 400px;
+    height: 400px;
+    background: green;
+    transform: translateX(110px);
+    clip: rect(0px, 200px, 200px, 0px);
+  }
+</style>
+
+<div>
+  <p>Expected: A green box.<br>
+  There should be no red visible.<br>
+  There should be a crisp, clipped edge around the green box (no blurring).</p>
+</div>
+
+<div id="testcase"><div></div></div>
+
+<div id="padding" style="height: 100vh"></div>
copy from testing/web-platform/tests/css/css-masking/clip/clip-filter-order.html
copy to testing/web-platform/tests/css/css-masking/clip/clip-transform-order.html
--- a/testing/web-platform/tests/css/css-masking/clip/clip-filter-order.html
+++ b/testing/web-platform/tests/css/css-masking/clip/clip-transform-order.html
@@ -1,25 +1,28 @@
-<!DOCTYPE html>
-<meta charset="utf-8">
-<title>Clip should be applied after filtering</title>
-<link rel="author" title="Philip Rogers" href="mailto:pdr@chromium.org">
+<!doctype html>
+<title>Clips should be applied before transforms</title>
 <link rel="help" href="https://drafts.fxtf.org/css-masking-1/#placement">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1524966">
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
 <link rel="match" href="clip-filter-order-ref.html">
 
+<style>
+  body { margin: 0 }
+  #testcase {
+    position: absolute;
+    left: -100px;
+    width: 400px;
+    height: 400px;
+    background: green;
+    transform: translateX(110px);
+    clip: rect(0px, 200px, 200px, 0px);
+  }
+</style>
+
 <div>
   <p>Expected: A green box.<br>
   There should be no red visible.<br>
   There should be a crisp, clipped edge around the green box (no blurring).</p>
 </div>
 
-<style>
-  #testcase {
-    position: absolute;
-    width: 400px;
-    height: 400px;
-    background: green;
-    will-change: transform;
-    filter: drop-shadow(16px 16px 20px red);
-    clip: rect(0px, 200px, 200px, 0px);
-  }
-</style>
-<div id="testcase"></div>
+<div id="testcase"><div></div></div>