Bug 138481 – SVG foreign objects do not inherit the container coordinates system if they are repainted

• Component: SVG
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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;

[Said Abou-Hallawa]

Created attachment 241227 [details]
Patch

[Simon Fraser (smfr)]

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.

[alan]

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.

[Said Abou-Hallawa]

Created attachment 241309 [details]
Patch

[Said Abou-Hallawa]

(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.

[Simon Fraser (smfr)]

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)

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 241317 [details]
Patch

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

(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.

[WebKit Commit Bot]

Comment on attachment 241317 [details]
Patch

Clearing flags on attachment: 241317

Committed r175847: <http://trac.webkit.org/changeset/175847>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.