From 416881eac99b14a753a1ee6f2fa26b6cb5be6e2d Mon Sep 17 00:00:00 2001 From: Tom Klein Date: Wed, 21 Nov 2018 00:08:11 +0000 Subject: [PATCH] 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 --HG-- extra : moz-landing-system : lando --- dom/svg/test/mochitest.ini | 1 + .../test/test_hit-testing-and-viewbox.xhtml | 82 +++++++++++++++++++ layout/base/RestyleManager.cpp | 56 ++++++++----- layout/base/nsChangeHint.h | 4 +- 4 files changed, 120 insertions(+), 23 deletions(-) create mode 100644 dom/svg/test/test_hit-testing-and-viewbox.xhtml diff --git a/dom/svg/test/mochitest.ini b/dom/svg/test/mochitest.ini index d32c2efef5f5..0f7115ff98f3 100644 --- a/dom/svg/test/mochitest.ini +++ b/dom/svg/test/mochitest.ini @@ -46,6 +46,7 @@ support-files = [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] diff --git a/dom/svg/test/test_hit-testing-and-viewbox.xhtml b/dom/svg/test/test_hit-testing-and-viewbox.xhtml new file mode 100644 index 000000000000..fd76eab20c68 --- /dev/null +++ b/dom/svg/test/test_hit-testing-and-viewbox.xhtml @@ -0,0 +1,82 @@ + + + + Test that hit-testing works after a viewBox update + + + + + + + + + +Mozilla Bug 1486952 +

+
+ +
+ + + + + + + +
+
+
+ + diff --git a/layout/base/RestyleManager.cpp b/layout/base/RestyleManager.cpp index a5769463d7f0..4a4fe83a53d5 100644 --- a/layout/base/RestyleManager.cpp +++ b/layout/base/RestyleManager.cpp @@ -667,11 +667,9 @@ static void SyncViewsAndInvalidateDescendants(nsIFrame* aFrame, 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 @@ -681,8 +679,7 @@ static void StyleChangeReflow(nsIFrame* aFrame, nsChangeHint aHint); * 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) @@ -1688,30 +1685,47 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList) 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); + } + } 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"); } - 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 diff --git a/layout/base/nsChangeHint.h b/layout/base/nsChangeHint.h index de94580cd18a..cf90beaf6509 100644 --- a/layout/base/nsChangeHint.h +++ b/layout/base/nsChangeHint.h @@ -111,8 +111,8 @@ enum nsChangeHint : uint32_t { 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,