Bug 1486952 - Update the overflow of the anonymous child when the viewBox changes. r=jwatt
☠☠ backed out by d43f21209617 ☠ ☠
authorTom Klein <twointofive@gmail.com>
Wed, 21 Nov 2018 00:08:11 +0000
changeset 503917 1c8f22180210bf31334c61b0c884ab8611ba7c15
parent 503916 9db1fb9190c7c5b9ba75b3ca83099b2a329c931f
child 503918 df7e7d55004ddbd34b26e1eab09bb0faa7500f5d
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1486952, 828240
milestone65.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 1486952 - Update the overflow of the anonymous child when the viewBox changes. r=jwatt Bug 828240 switched the children only transform from applying to each of the anonymous child's children to applying directly to the anonymous child instead. So now when the viewBox changes, we need to update (just) the overflow of the anonymous child, where previously we were updating the overflows of each of the children (which was having no effect I guess, since they no longer have the child only transform applied to them). Hit testing checks for hits using overflow, so was broken by the lack of overflow update on the anonymous child (combined with the fact that the anonymous child is now transformed by the children only transform). Differential Revision: https://phabricator.services.mozilla.com/D5668
dom/svg/test/mochitest.ini
dom/svg/test/test_hit-testing-and-viewbox.xhtml
layout/base/RestyleManager.cpp
layout/base/nsChangeHint.h
--- a/dom/svg/test/mochitest.ini
+++ b/dom/svg/test/mochitest.ini
@@ -41,16 +41,17 @@ support-files =
 [test_getBBox-method.html]
 [test_dataTypes.html]
 [test_dataTypesModEvents.html]
 [test_fragments.html]
 [test_getCTM.html]
 [test_getElementById.xhtml]
 [test_getSubStringLength.xhtml]
 [test_getTotalLength.xhtml]
+[test_hit-testing-and-viewbox.xhtml]
 [test_lang.xhtml]
 skip-if = true # disabled-for-intermittent-failures--bug-701060
 [test_length.xhtml]
 [test_lengthParsing.html]
 [test_markerOrient.xhtml]
 [test_nonAnimStrings.xhtml]
 [test_non-scaling-stroke.html]
 [test_object-delayed-intrinsic-size.html]
new file mode 100644
--- /dev/null
+++ b/dom/svg/test/test_hit-testing-and-viewbox.xhtml
@@ -0,0 +1,82 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1486952
+-->
+<head>
+  <title>Test that hit-testing works after a viewBox update</title>
+
+  <style>
+    :hover { fill: lime; }
+  </style>
+
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+
+<body onload="run()">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+
+SimpleTest.waitForExplicitFinish();
+
+function run()
+{
+  const div = $('div');
+  const offsetX = div.offsetLeft;
+  const offsetY = div.offsetTop;
+  const outerRect = $('outerRect');
+  const innerRect = $('innerRect');
+  const outerSVG = $('outerSVG');
+  const innerSVG = $('innerSVG');
+  let got;
+
+  // Update the inner SVG viewBox to "move" the inner rectangle off its current
+  // position on screen:
+  innerSVG.setAttribute('viewBox', '-50 0 100 100');
+  got = document.elementFromPoint(offsetX + 300, offsetY + 50);
+  is(got, innerRect, 'Should hit inner rectangle (1)');
+
+  // Update the inner SVG viewBox again. (At the time of writing, a reflow is
+  // triggered the first time you change viewBox on an inner svg, so the
+  // previous test is expected to always pass. This next test will fail if we're
+  // updating overflows on the inner svg frame instead of its children).
+  innerSVG.setAttribute('viewBox', '0 -50 100 100');
+  got = document.elementFromPoint(offsetX + 200, offsetY + 150);
+  is(got, innerRect, 'Should hit inner rectangle (2)');
+
+  // Now update the outer SVG viewBox and confirm that both rectangles are
+  // registered.  (Note that in this case the overflow rectangle of the inner
+  // svg is the inner svg's viewport, so be sure to "move" the outer svg so that
+  // the "new" outer rectangle and inner svg are off the current outerRect
+  // union inner svg viewport - hit testing still works in that union.)
+  outerSVG.setAttribute('viewBox', '-400 0 800 200');
+  // Outer:
+  got = document.elementFromPoint(offsetX + 500, offsetY + 100);
+  is(got, outerRect, 'Should hit outer rectangle');
+  // Inner:
+  got = document.elementFromPoint(offsetX + 600, offsetY + 150);
+  is(got, innerRect, 'Should hit inner rectangle (3)');
+
+  SimpleTest.finish();
+}
+
+]]>
+</script>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1486952">Mozilla Bug 1486952</a>
+<p id="display"></p>
+<div id="content">
+
+  <div width="100%" height="1" id="div"></div>
+  <svg xmlns="http://www.w3.org/2000/svg" id="outerSVG" width="800" height="200"
+       viewBox="0 0 800 200">
+    <rect x="50" y="50" width="100" height="100" fill="red" id="outerRect" />
+    <svg x="150" width="200" height="200" viewBox="0 0 100 100" id="innerSVG">
+      <rect width="50" height="50" fill="red" id="innerRect" />
+    </svg>
+  </svg>
+
+</div>
+<pre id="test">
+</pre>
+</body>
+</html>
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -662,32 +662,29 @@ static bool gInApplyRenderingChangeToTre
  * nsChangeHint_SchedulePaint, nothing else.
 */
 static void SyncViewsAndInvalidateDescendants(nsIFrame* aFrame,
                                               nsChangeHint aChange);
 
 static void StyleChangeReflow(nsIFrame* aFrame, nsChangeHint aHint);
 
 /**
- * To handle nsChangeHint_ChildrenOnlyTransform we must iterate over the child
- * frames of the SVG frame concerned. This helper function is used to find that
- * SVG frame when we encounter nsChangeHint_ChildrenOnlyTransform to ensure
- * that we iterate over the intended children, since sometimes we end up
- * handling that hint while processing hints for one of the SVG frame's
+ * This helper function is used to find the correct SVG frame to target when we
+ * encounter nsChangeHint_ChildrenOnlyTransform; needed since sometimes we end
+ * up handling that hint while processing hints for one of the SVG frame's
  * ancestor frames.
  *
  * The reason that we sometimes end up trying to process the hint for an
  * ancestor of the SVG frame that the hint is intended for is due to the way we
  * process restyle events. ApplyRenderingChangeToTree adjusts the frame from
  * the restyled element's principle frame to one of its ancestor frames based
  * on what nsCSSRendering::FindBackground returns, since the background style
  * may have been propagated up to an ancestor frame. Processing hints using an
  * ancestor frame is fine in general, but nsChangeHint_ChildrenOnlyTransform is
- * a special case since it is intended to update the children of a specific
- * frame.
+ * a special case since it is intended to update a specific frame.
  */
 static nsIFrame*
 GetFrameForChildrenOnlyTransformHint(nsIFrame* aFrame)
 {
   if (aFrame->IsViewportFrame()) {
     // This happens if the root-<svg> is fixed positioned, in which case we
     // can't use aFrame->GetContent() to find the primary frame, since
     // GetContent() returns nullptr for ViewportFrame.
@@ -1683,40 +1680,57 @@ RestyleManager::ProcessRestyledFrames(ns
             AddSubtreeToOverflowTracker(cont, mOverflowChangedTracker);
           }
           // The work we just did in AddSubtreeToOverflowTracker
           // subsumes some of the other hints:
           hint &= ~(nsChangeHint_UpdateOverflow |
                     nsChangeHint_UpdatePostTransformOverflow);
         }
         if (hint & nsChangeHint_ChildrenOnlyTransform) {
-          // The overflow areas of the child frames need to be updated:
+          // We need to update overflows. The correct frame(s) to update depends
+          // on whether the ChangeHint came from an outer or an inner svg.
           nsIFrame* hintFrame = GetFrameForChildrenOnlyTransformHint(frame);
-          nsIFrame* childFrame = hintFrame->PrincipalChildList().FirstChild();
           NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrIBSplitSibling(frame),
                        "SVG frames should not have continuations "
                        "or ib-split siblings");
           NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrIBSplitSibling(hintFrame),
                        "SVG frames should not have continuations "
                        "or ib-split siblings");
-          for ( ; childFrame; childFrame = childFrame->GetNextSibling()) {
-            MOZ_ASSERT(childFrame->IsFrameOfType(nsIFrame::eSVG),
-                       "Not expecting non-SVG children");
-            // If |childFrame| is dirty or has dirty children, we don't bother
+          if (hintFrame->IsSVGOuterSVGAnonChildFrame()) {
+            // The children only transform of an outer svg frame is applied to
+            // the outer svg's anonymous child frame (instead of to the
+            // anonymous child's children).
+
+            // If |hintFrame| is dirty or has dirty children, we don't bother
             // updating overflows since that will happen when it's reflowed.
-            if (!(childFrame->GetStateBits() &
+            if (!(hintFrame->GetStateBits() &
                   (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
-              mOverflowChangedTracker.AddFrame(childFrame,
-                                        OverflowChangedTracker::CHILDREN_CHANGED);
+              mOverflowChangedTracker.AddFrame(hintFrame,
+                                       OverflowChangedTracker::CHILDREN_CHANGED);
             }
-            NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrIBSplitSibling(childFrame),
-                         "SVG frames should not have continuations "
-                         "or ib-split siblings");
-            NS_ASSERTION(childFrame->GetParent() == hintFrame,
-                         "SVG child frame not expected to have different parent");
+          } else {
+            // The children only transform is applied to the child frames of an
+            // inner svg frame, so update the child overflows.
+            nsIFrame* childFrame = hintFrame->PrincipalChildList().FirstChild();
+            for ( ; childFrame; childFrame = childFrame->GetNextSibling()) {
+              MOZ_ASSERT(childFrame->IsFrameOfType(nsIFrame::eSVG),
+                         "Not expecting non-SVG children");
+              // If |childFrame| is dirty or has dirty children, we don't bother
+              // updating overflows since that will happen when it's reflowed.
+              if (!(childFrame->GetStateBits() &
+                    (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
+                mOverflowChangedTracker.AddFrame(childFrame,
+                                       OverflowChangedTracker::CHILDREN_CHANGED);
+              }
+              NS_ASSERTION(!nsLayoutUtils::GetNextContinuationOrIBSplitSibling(childFrame),
+                           "SVG frames should not have continuations "
+                           "or ib-split siblings");
+              NS_ASSERTION(childFrame->GetParent() == hintFrame,
+                        "SVG child frame not expected to have different parent");
+            }
           }
         }
         // If |frame| is dirty or has dirty children, we don't bother updating
         // overflows since that will happen when it's reflowed.
         if (!(frame->GetStateBits() &
               (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN))) {
           if (hint & (nsChangeHint_UpdateOverflow |
                       nsChangeHint_UpdatePostTransformOverflow)) {
--- a/layout/base/nsChangeHint.h
+++ b/layout/base/nsChangeHint.h
@@ -106,18 +106,18 @@ enum nsChangeHint : uint32_t {
    * This frame's effect on its parent's overflow area has changed.
    * (But neither its pre-transform nor post-transform overflow have
    * changed; if those are the case, see
    * nsChangeHint_UpdatePostTransformOverflow.)
    */
   nsChangeHint_UpdateParentOverflow = 1 << 14,
 
   /**
-   * The children-only transform of an SVG frame changed, requiring the
-   * overflow rects of the frame's immediate children to be updated.
+   * The children-only transform of an SVG frame changed, requiring overflows to
+   * be updated.
    */
   nsChangeHint_ChildrenOnlyTransform = 1 << 15,
 
   /**
    * The frame's offsets have changed, while its dimensions might have
    * changed as well.  This hint is used for positioned frames if their
    * offset changes.  If we decide that the dimensions are likely to
    * change, this will trigger a reflow.