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
Created attachment 98197 [details] Test case
Created attachment 231333 [details] Patch
Created attachment 231335 [details] Patch
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?
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
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
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
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
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
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
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
Created attachment 231359 [details] Patch for landing
Created attachment 231360 [details] Patch for landing
(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.
Comment on attachment 231360 [details] Patch for landing Clearing flags on attachment: 231360 Committed r168674: <http://trac.webkit.org/changeset/168674>
All reviewed patches have been landed. Closing bug.
(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.
(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!