Bug 63153 - SVG root element accepts background color but fails to repaint it
Summary: SVG root element accepts background color but fails to repaint it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-22 10:23 PDT by Tim Horton
Modified: 2014-05-13 00:16 PDT (History)
15 users (show)

See Also:


Attachments
Test case (1.39 KB, application/xhtml+xml)
2011-06-22 10:23 PDT, Tim Horton
no flags Details
Patch (10.53 KB, patch)
2014-05-12 15:33 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (10.76 KB, patch)
2014-05-12 15:49 PDT, Dirk Schulze
dino: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (1.23 MB, application/zip)
2014-05-12 17:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (811.08 KB, application/zip)
2014-05-12 18:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (875.91 KB, application/zip)
2014-05-12 18:31 PDT, Build Bot
no flags Details
Patch for landing (32.89 KB, patch)
2014-05-12 21:00 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch for landing (33.92 KB, patch)
2014-05-12 22:06 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-06-22 10:23:01 PDT
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
Comment 1 Tim Horton 2011-06-22 10:23:20 PDT
Created attachment 98197 [details]
Test case
Comment 2 Dirk Schulze 2014-05-12 15:33:51 PDT
Created attachment 231333 [details]
Patch
Comment 3 Dirk Schulze 2014-05-12 15:49:18 PDT
Created attachment 231335 [details]
Patch
Comment 4 Dean Jackson 2014-05-12 16:10:26 PDT
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 5 Dean Jackson 2014-05-12 16:10:50 PDT
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 6 Build Bot 2014-05-12 17:00:56 PDT
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
Comment 7 Build Bot 2014-05-12 17:01:03 PDT
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 8 Build Bot 2014-05-12 18:29:25 PDT
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
Comment 9 Build Bot 2014-05-12 18:29:30 PDT
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 10 Build Bot 2014-05-12 18:31:42 PDT
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
Comment 11 Build Bot 2014-05-12 18:31:47 PDT
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
Comment 12 Dirk Schulze 2014-05-12 21:00:21 PDT
Created attachment 231359 [details]
Patch for landing
Comment 13 Dirk Schulze 2014-05-12 22:06:54 PDT
Created attachment 231360 [details]
Patch for landing
Comment 14 Philip Rogers 2014-05-12 23:00:31 PDT
(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 15 WebKit Commit Bot 2014-05-12 23:05:31 PDT
Comment on attachment 231360 [details]
Patch for landing

Clearing flags on attachment: 231360

Committed r168674: <http://trac.webkit.org/changeset/168674>
Comment 16 WebKit Commit Bot 2014-05-12 23:05:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Dirk Schulze 2014-05-12 23:56:08 PDT
(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.
Comment 18 Dirk Schulze 2014-05-13 00:16:02 PDT
(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!