Bug 63153 – SVG root element accepts background color but fails to repaint it

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

Steps to reproduce:

1. Open the attached example in a WebKit browser.
2. Click the button a few times.

Expected outcome:

The yellow left square becomes consistently bigger, and the innards are left alone once they reach square form; the yellow right square becomes consistently smaller, clipping the innards as it shrinks.

Actual outcome:

The yellow left square fails to fill in (though the innards do appropriately redraw), and the yellow right square fails to be cleared in some areas.

Reasoning:

This seems to occur because of two things:

1. The test case uses CSS background-color on a toplevel <svg> element, which is apparently [1] undefined behaviour.

2. In r62922, we switched to redrawing only the contents of an SVG, instead of the entire element, so now, any changes made outside the bounding rect of the children of the toplevel <svg> element won't be redrawn properly.

Conclusion:

I don't think we should support background-color on <svg> until SVG 2.0, but I wanted confirmation from others before making that decision. Certainly, ignoring it outright like we do *fill* on <svg> is better than letting it happen but corrupting it when changes are made.




[1] http://lists.w3.org/Archives/Public/www-svg/2010Aug/0050.html

[Tim Horton]

Created attachment 98197 [details]
Test case

[Dirk Schulze]

Created attachment 231333 [details]
Patch

[Dirk Schulze]

Created attachment 231335 [details]
Patch

[Dean Jackson]

Comment on attachment 231335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231335&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:193
> +bool RenderBoxModelObject::calculateHasBoxDecorations() const

I don't like this name. What is being "calculated"? It's not returning a number.

If you keep this name, then I think it should return void and set m_hasBoxDecorations, or something like that. I guess it depends on how you are using it.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:240
> +    // SVG outlines are painted during PaintPhaseForeground.
> +    if (paintInfo.phase == PaintPhaseOutline || paintInfo.phase == PaintPhaseSelfOutline)
> +        return;
> +

Is that from the previous patch?

[Dean Jackson]

Comment on attachment 231335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231335&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:378
> +    // return SVGRenderSupport::clippedOverflowRectForRepaint(*this, repaintContainer);

oops

[Build Bot]

Comment on attachment 231335 [details]
Patch

Attachment 231335 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4631502360936448

New failing tests:
svg/animations/animate-viewport-overflow.html
svg/text/small-fonts-in-html5.html
svg/custom/pattern-userSpaceOnUse-userToBaseTransform.xhtml
svg/custom/mouse-move-on-svg-root-standalone.svg
svg/repaint/repaint-webkit-svg-shadow.svg
svg/custom/mouse-move-on-svg-container-standalone.svg
svg/css/composite-shadow-example.html
svg/custom/circle-move-invalidation.svg
svg/hixie/mixed/010.xml
svg/css/composite-shadow-with-opacity.html
fast/repaint/moving-shadow-on-container.html
svg/custom/mouse-move-on-svg-root.xhtml
svg/custom/gradient-stroke-width.svg
svg/custom/mouse-move-on-svg-container.xhtml
svg/custom/simple-text-double-shadow.svg
svg/animations/animate-viewport-overflow-2.html

[Build Bot]

Created attachment 231345 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231335 [details]
Patch

Attachment 231335 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5264458402234368

New failing tests:
svg/animations/animate-viewport-overflow.html
svg/text/small-fonts-in-html5.html
svg/custom/pattern-userSpaceOnUse-userToBaseTransform.xhtml
svg/custom/mouse-move-on-svg-root-standalone.svg
svg/repaint/repaint-webkit-svg-shadow.svg
svg/custom/mouse-move-on-svg-container-standalone.svg
svg/css/composite-shadow-example.html
svg/custom/circle-move-invalidation.svg
svg/hixie/mixed/010.xml
svg/css/composite-shadow-with-opacity.html
fast/repaint/moving-shadow-on-container.html
svg/custom/mouse-move-on-svg-root.xhtml
svg/custom/gradient-stroke-width.svg
svg/custom/mouse-move-on-svg-container.xhtml
svg/custom/simple-text-double-shadow.svg
svg/animations/animate-viewport-overflow-2.html

[Build Bot]

Created attachment 231352 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231335 [details]
Patch

Attachment 231335 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6579984604856320

New failing tests:
svg/animations/animate-viewport-overflow.html
svg/text/small-fonts-in-html5.html
svg/custom/pattern-userSpaceOnUse-userToBaseTransform.xhtml
svg/custom/mouse-move-on-svg-root-standalone.svg
svg/repaint/repaint-webkit-svg-shadow.svg
svg/custom/mouse-move-on-svg-container-standalone.svg
svg/css/composite-shadow-example.html
svg/custom/circle-move-invalidation.svg
svg/hixie/mixed/010.xml
svg/css/composite-shadow-with-opacity.html
fast/repaint/moving-shadow-on-container.html
svg/custom/mouse-move-on-svg-root.xhtml
svg/custom/gradient-stroke-width.svg
svg/custom/mouse-move-on-svg-container.xhtml
svg/custom/simple-text-double-shadow.svg
svg/animations/animate-viewport-overflow-2.html

[Build Bot]

Created attachment 231353 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dirk Schulze]

Created attachment 231359 [details]
Patch for landing

[Dirk Schulze]

Created attachment 231360 [details]
Patch for landing

[Philip Rogers]

(In reply to comment #13)
> Created an attachment (id=231360) [details]
> Patch for landing

Please do not blindly merge Blink patches without the proper investigation. Git blame in Blink shows the correct revision is https://src.chromium.org/viewvc/blink?revision=170960&view=revision, and shows that this merge was incomplete.

Oilpan is landing in Blink as we speak and merges between the projects will soon be dangerous due to the different lifetimes of objects. It's very important that we fully investigate and understand merges to avoid correctness and security regressions.

[WebKit Commit Bot]

Comment on attachment 231360 [details]
Patch for landing

Clearing flags on attachment: 231360

Committed r168674: <http://trac.webkit.org/changeset/168674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=231360) [details] [details]
> > Patch for landing
> 
> Please do not blindly merge Blink patches without the proper investigation. Git blame in Blink shows the correct revision is https://src.chromium.org/viewvc/blink?revision=170960&view=revision, and shows that this merge was incomplete.
> 
> Oilpan is landing in Blink as we speak and merges between the projects will soon be dangerous due to the different lifetimes of objects. It's very important that we fully investigate and understand merges to avoid correctness and security regressions.

I did not. If there is something I missed, please upload a test case.

[Dirk Schulze]

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=231360) [details] [details]
> > Patch for landing
> 
> Please do not blindly merge Blink patches without the proper investigation. Git blame in Blink shows the correct revision is https://src.chromium.org/viewvc/blink?revision=170960&view=revision, and shows that this merge was incomplete.
> 
> Oilpan is landing in Blink as we speak and merges between the projects will soon be dangerous due to the different lifetimes of objects. It's very important that we fully investigate and understand merges to avoid correctness and security regressions.

Talked with pdr a couple of minutes ago. He had concerns that I merged a problem with overflowing behavior as well. Bit of context: Just recently we added support for more than just overflow: hidden on root SVG elements. The Blink patch didn't take this into account.

I didn't merge the part in question but used a different approach. I am not saying that there is no issue (there always can be) but the situation might differ to Blink.

Thanks for checking pdr!