Created attachment 241137 [details] Testcase In attached testcase, hovering the blue box should make it turn green. What happens is that only part of the box is repainted.
RenderSVG* classes use computeFloatRectForRepaint(), but RenderBox etc use computeRectForRepaint(). You'd think that RenderSVGForeignObject would jump from the latter to the former, but it doesn't, so we end up hitting RenderObject::computeRectForRepaint() for a RenderSVGTransformableContainer and completely miss the transform.
This fixes it: diff --git a/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp b/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp index d423695704e73f6340b789941d9fcd657a05a720..13a08c35a3af6ff04b734924c972ea6d488b0285 100644 --- a/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp +++ b/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp @@ -102,6 +102,14 @@ void RenderSVGForeignObject::computeFloatRectForRepaint(const RenderLayerModelOb SVGRenderSupport::computeFloatRectForRepaint(*this, repaintContainer, repaintRect, fixed); } +// Map from HTML-style (computeRectForRepaint) to SVG-style (computeFloatRectForRepaint). +void RenderSVGForeignObject::computeRectForRepaint(const RenderLayerModelObject* repaintContainer, LayoutRect& repaintRect, bool fixed) const +{ + FloatRect floatRect(repaintRect); + computeFloatRectForRepaint(repaintContainer, floatRect, fixed); + repaintRect = enclosingLayoutRect(floatRect); +} + const AffineTransform& RenderSVGForeignObject::localToParentTransform() const { m_localToParentTransform = localTransform(); diff --git a/Source/WebCore/rendering/svg/RenderSVGForeignObject.h b/Source/WebCore/rendering/svg/RenderSVGForeignObject.h index c75bbff14da489428cb97f8d407dae4c006c47d3..86a534d976b70b67345a8f55f0df32b8a1ace9b1 100644 --- a/Source/WebCore/rendering/svg/RenderSVGForeignObject.h +++ b/Source/WebCore/rendering/svg/RenderSVGForeignObject.h @@ -41,6 +41,7 @@ public: virtual LayoutRect clippedOverflowRectForRepaint(const RenderLayerModelObject* repaintContainer) const override; virtual void computeFloatRectForRepaint(const RenderLayerModelObject* repaintContainer, FloatRect&, bool fixed = false) const override; + virtual void computeRectForRepaint(const RenderLayerModelObject* repaintContainer, LayoutRect&, bool fixed = false) const override; virtual bool requiresLayer() const override { return false; } virtual void layout() override;
Created attachment 241227 [details] Patch
Comment on attachment 241227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241227&action=review > LayoutTests/ChangeLog:13 > + * svg/in-html/svg-foreign-object-percentage-expected.html: Added. > + * svg/in-html/svg-foreign-object-percentage.html: Added. Is this test detecting a behavior change introduced by the patch? That seems odd given that I don't think this patch affects layout. > LayoutTests/ChangeLog:20 > + Re-baseline these tests since the expected render tree was incorrect although > + the rendering itself did not change. > + * platform/mac/svg/zoom/page/zoom-foreignObject-expected.txt: > + * svg/zoom/page/zoom-foreign-content-expected.txt: Did these really change because of the patch? The patch should only affect repaints, not the size of renderers. > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:25 > + document.body.offsetWidth; // forces layout You don't need to force layout here. We always layout at document load. > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:29 > + document.body.offsetWidth; // forces layout You don't need to force layout here either; we always layout at the end of the test.
Comment on attachment 241227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241227&action=review > Source/WebCore/ChangeLog:9 > + of the container if it is repainted. The foreign object is actually redrawn in the ". The" < extra space. > Source/WebCore/ChangeLog:14 > + The bug is RenderSVG* classes use computeFloatRectForRepaint(), but RenderBox etc "RenderBox etc" ? > Source/WebCore/ChangeLog:23 > + Implement RenderSVGForeignObject::computeRectForRepaint() so it can inherit > + the container transformation. what does "inherit the container transformation" mean in the context of computeRectForRepaint? > Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp:105 > +// Map from HTML-style (computeRectForRepaint) to SVG-style (computeFloatRectForRepaint). What do HTML-style and SVG-style mean? > LayoutTests/ChangeLog:15 > + * svg/transforms/svg-transform-foreign-object-repaint-expected.html: Added. > + * svg/transforms/svg-transform-foreign-object-repaint.html: Added. Alternatively you could use startTrackingRepaints/stopTrackingRepaints test framework APIs, but probably the reftest is better.
Created attachment 241309 [details] Patch
(In reply to comment #4) > Comment on attachment 241227 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241227&action=review > > > LayoutTests/ChangeLog:13 > > + * svg/in-html/svg-foreign-object-percentage-expected.html: Added. > > + * svg/in-html/svg-foreign-object-percentage.html: Added. > > Is this test detecting a behavior change introduced by the patch? That seems > odd given that I don't think this patch affects layout. > I was trying Safari release 10600.1.25 which has the bug of not showing foreign objects with percentage size. I tried it with the fix and it was fixed. I thought it was fixed because of this patch. But when I tried without the patch, I found it is still fixed. So you are right. This test is unrelated and I removed it. > > LayoutTests/ChangeLog:20 > > + Re-baseline these tests since the expected render tree was incorrect although > > + the rendering itself did not change. > > + * platform/mac/svg/zoom/page/zoom-foreignObject-expected.txt: > > + * svg/zoom/page/zoom-foreign-content-expected.txt: > > Did these really change because of the patch? The patch should only affect > repaints, not the size of renderers. > Yes. But I think the render tree dump was calling computeRectForRepaint() of the base class and were missing all the transformation. The SVG rendering itself uses the computeFloatRectForRepaint(). > > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:25 > > + document.body.offsetWidth; // forces layout > > You don't need to force layout here. We always layout at document load. > Done. I am using the startTrackingRepaints()/stopTrackingRepaints() test framework API along with a reference image. > > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:29 > > + document.body.offsetWidth; // forces layout > > You don't need to force layout here either; we always layout at the end of > the test. Done.
Comment on attachment 241309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241309&action=review > Source/WebCore/ChangeLog:12 > + container. The bug only happens if the foreign object is repainted after the first > + time the page is loaded. "The bug only happens if"... Dynamic changes in web content are the norm, not the exception, so I don't think this sentence adds anything. > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:26 > + elements[0].style.background = "red"; I would prefer having a classname for the changed style, like: .changed { background-color: green; } and here to use elements[0].classList.add('changed'); Please don't use red in a passing test. Red indicates failure. Please also make it so that the test result doesn't have a scrollbar. We've had scrollbar issues in the past. > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:46 > + setTimeout(changeBackground, 20); I think this can be setTimeout(, 0)
Comment on attachment 241309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241309&action=review > LayoutTests/ChangeLog:19 > + Re-baseline these tests since we were getting the the rectangles of the > + RenderSVGTransformableContainer through the base class method > + RenderObject::computeRectForRepaint() and we were completely missing the > + transform. The display was fine since we call computeFloatRectForRepaint() > + for rendering SVG elements. This explanation isn't quite right. The issue is that the render tree dumping uses renderer.absoluteClippedOverflowRect() in writePositionAndStyle(TextStream& ts, const RenderElement& renderer) so this issue only affects test output.
Created attachment 241317 [details] Patch
(In reply to comment #8) > Comment on attachment 241309 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241309&action=review > > > Source/WebCore/ChangeLog:12 > > + container. The bug only happens if the foreign object is repainted after the first > > + time the page is loaded. > > "The bug only happens if"... > Dynamic changes in web content are the norm, not the exception, so I don't > think this sentence adds anything. > Done. I just wanted to emphasize the fact that this bug is a repaint issue. After the the page is painted for the first time any event that causes the page to repainted, the bug would happen. > > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:26 > > + elements[0].style.background = "red"; > > I would prefer having a classname for the changed style, like: > > .changed { > background-color: green; > } > and here to use elements[0].classList.add('changed'); > > Please don't use red in a passing test. Red indicates failure. > Done. > Please also make it so that the test result doesn't have a scrollbar. We've > had scrollbar issues in the past. > Done. > > LayoutTests/svg/transforms/svg-transform-foreign-object-repaint.html:46 > > + setTimeout(changeBackground, 20); > > I think this can be setTimeout(, 0) I could not get the setTimeout to 0. The reason was the test was flaky with and without the fix. I was getting the expected and the unexpected results when I refresh the page very fast.
(In reply to comment #9) > Comment on attachment 241309 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241309&action=review > > > LayoutTests/ChangeLog:19 > > + Re-baseline these tests since we were getting the the rectangles of the > > + RenderSVGTransformableContainer through the base class method > > + RenderObject::computeRectForRepaint() and we were completely missing the > > + transform. The display was fine since we call computeFloatRectForRepaint() > > + for rendering SVG elements. > > This explanation isn't quite right. The issue is that the render tree > dumping uses renderer.absoluteClippedOverflowRect() in > writePositionAndStyle(TextStream& ts, const RenderElement& renderer) so this > issue only affects test output. This is correct. Thanks for the hint.
Comment on attachment 241317 [details] Patch Clearing flags on attachment: 241317 Committed r175847: <http://trac.webkit.org/changeset/175847>
All reviewed patches have been landed. Closing bug.