Bug 5358 – The outermost <svg> element needs to clip itself

• Component: SVG
• Status: RESOLVED FIXED

[Julien Palmas]

---SVG1---

<svg width="300" height="200" viewBox="-100 -100 300 200">
  <rect x="0" y="0" width="300" height="200" fill="yellow" stroke="blue"/>
</svg>

---SVG2---

<svg width="300" height="200">
  <rect x="100" y="100" width="300" height="200" fill="yellow" stroke="blue"/>
</svg>

SVG1 and SVG2 should render identically. The yellow rectangle must be clipped.
Right now, SVG2 renders correctly, but SVG1 doesn't. This is due to the wrong data values of the 
KCanvasItem element.

Here is DumpKCanvasTree output for SVG1:

KCanvasRegistry: empty
KCanvasContainer at (99,99) size 301x201
  KCanvasItem {rect} at (99,99) size 301x201 [stroke={[type=SOLID] [color=#0000FF]}] [fill=
{[type=SOLID] [color=#FFFF00]}] [data="M0.00,0.00L300.00,0.00L300.00,200.00L0.00,200.00"]

Here is DumpKCanvasTree output for SVG1:

KCanvasRegistry: empty
KCanvasContainer at (99,99) size 301x201
  KCanvasItem {rect} at (99,99) size 301x201 [stroke={[type=SOLID] [color=#0000FF]}] [fill=
{[type=SOLID] [color=#FFFF00]}] 
[data="M100.00,100.00L400.00,100.00L400.00,300.00L100.00,300.00"]

Notice the difference in "data".

[Julien Palmas]

Sorry .... The first post needs modifications.

Actually, both SVG1 and SVG2 render incorrectly. They look the same, even though they don't have the 
same "data" values (don't know why by the way ...).

One big rendering mistake is that the yellow rect is not clipped (for both SVG1 and SVG2).
This is because the KCanvasContainer coordinates are wrong.
Right now, it looks like: KCanvasContainer at (99,99) size 301x201
And it should be like: KCanvasContainer at (0,0) size 300x200

[Julien Palmas]

Created attachment 4333 [details]
Wrong rendering

[Julien Palmas]

Created attachment 4334 [details]
Correct rendering

[Eric Seidel (no email)]

It would be even easier if those were attached as .svg attachments (thus could be viewed inline).

[Julien Palmas]

Created attachment 4341 [details]
SVG1 example

[Julien Palmas]

Created attachment 4342 [details]
SVG2 example

[Julien Palmas]

Created attachment 4363 [details]
SVG2 example

The former example was missing a ">" for the closing svg element

[Eric Seidel (no email)]

The attached SVGs no longer render due to the lack of SVGDocumentImpl and <svg> not being valid in 
plain old XML.  http://bugzilla.opendarwin.org/show_bug.cgi?id=5978

[Julien Palmas]

Created attachment 5040 [details]
Overflow example

[Eric Seidel (no email)]

*** Bug 6093 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Ok, I tried to implement this tonight by allowing all <svg> elements to use layers.  I've since decided this was the wrong approach.  Why?  Well, the only reason that an <svg> element would ever *need* a layer is for absolute positioning in a compound document.  Using a layer at any other time is wrong, because of double-opacity (which has already been hacked around), lack of transforms, and improper clipping.

I've made some benificial adjustments to the code during this process, but I'm now going to go back and only allow the outer-most <svg> elements to have a layer, and only if they are absolutely positioned.

[Eric Seidel (no email)]

Created attachment 9976 [details]
Patch to finally fix this!

[Eric Seidel (no email)]

Created attachment 9994 [details]
patch showing additional test case

[Anders Carlsson]

Comment on attachment 9976 [details]
Patch to finally fix this!

#if SVG_SUPPORT should be #ifdef SVG_SUPPORT since we compile with -wundef now (which will generate an error if SVG_SUPPORT is undefined)

Looks great otherwise, r=me

[David Kilzer (:ddkilzer)]

This patch caused regressions (DRT crashes) in LayoutTests/css1.

http://build.webkit.org/post-commit-powerpc-mac-os-x/builds/2960/step-layout-test/0

Thread 0 Crashed:
0   com.apple.WebCore           0x011c7258 WebCore::CSSStyleSelector::adjustRenderStyle(WebCore::RenderStyle*, WebCore::Element*) + 3004 (cssstyleselector.cpp:1091)
1   com.apple.WebCore           0x011d75d0 WebCore::CSSStyleSelector::pseudoStyleForElement(WebCore::RenderStyle::PseudoId, WebCore::Element*, WebCore::RenderStyle*) + 904 (cssstyleselector.cpp:965)
2   com.apple.WebCore           0x0123e138 WebCore::RenderObject::getPseudoStyle(WebCore::RenderStyle::PseudoId, WebCore::RenderStyle*) const + 392 (RenderObject.cpp:2694)
3   com.apple.WebCore           0x0123e288 WebCore::RenderObject::firstLineStyle() const + 220 (RenderObject.cpp:2659)
4   com.apple.WebCore           0x011ff408 WebCore::RenderBlock::updateFirstLetter() + 1060 (RenderBlock.cpp:3428)
5   com.apple.WebCore           0x0123c7b8 WebCore::RenderObject::recalcMinMaxWidths() + 156 (RenderObject.cpp:2581)
6   com.apple.WebCore           0x0123c8b4 WebCore::RenderObject::recalcMinMaxWidths() + 408 (RenderObject.cpp:2593)
7   com.apple.WebCore           0x0123c8b4 WebCore::RenderObject::recalcMinMaxWidths() + 408 (RenderObject.cpp:2593)
8   com.apple.WebCore           0x0123c8b4 WebCore::RenderObject::recalcMinMaxWidths() + 408 (RenderObject.cpp:2593)
9   com.apple.WebCore           0x0120ddf0 WebCore::RenderView::layout() + 248 (RenderView.cpp:119)
10  com.apple.WebCore           0x01144b50 WebCore::FrameView::layout(bool) + 2296 (FrameView.cpp:476)
11  com.apple.WebCore           0x0114f1ac WebCore::Document::implicitClose() + 1300 (Document.cpp:1245)
12  com.apple.WebCore           0x0111a27c WebCore::Frame::checkEmitLoadEvent() + 724 (Frame.cpp:836)
13  com.apple.WebCore           0x0111f140 WebCore::Frame::checkCompleted() + 528 (Frame.cpp:801)
14  com.apple.WebCore           0x0111f544 WebCore::Frame::loadDone() + 108 (Frame.cpp:770)
15  com.apple.WebCore           0x01175558 WebCore::DocLoader::setLoadInProgress(bool) + 72 (DocLoader.cpp:207)
16  com.apple.WebCore           0x011779e4 WebCore::Loader::receivedAllData(WebCore::ResourceLoader*, NSData*) + 488 (loader.cpp:141)
17  com.apple.WebCore           0x0106268c -[WebCoreResourceLoaderImp finishJobAndHandle:] + 180 (WebCoreResourceLoaderImp.mm:98)
18  com.apple.WebCore           0x0106293c -[WebCoreResourceLoaderImp finishWithData:] + 196 (WebCoreResourceLoaderImp.mm:130)
19  com.apple.WebKit            0x002eb0e8 -[WebSubresourceLoader didFinishLoading] + 132 (WebSubresourceLoader.m:213)
20  com.apple.WebKit            0x002e6a68 -[WebLoader connectionDidFinishLoading:] + 184 (WebLoader.m:575)
21  com.apple.Foundation        0x9297684c -[NSURLConnection(NSURLConnectionInternal) _sendDidFinishLoadingCallback] + 188
22  com.apple.Foundation        0x92974ab8 -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] + 556
23  com.apple.Foundation        0x92974810 _sendCallbacks + 156
24  com.apple.CoreFoundation    0x907dc4cc __CFRunLoopDoSources0 + 384
25  com.apple.CoreFoundation    0x907db9fc __CFRunLoopRun + 452
26  com.apple.CoreFoundation    0x907db47c CFRunLoopRunSpecific + 268
27  com.apple.Foundation        0x92953164 -[NSRunLoop runMode:beforeDate:] + 172
28  DumpRenderTree              0x0000ab58 dumpRenderTree + 1056 (DumpRenderTree.m:766)
29  DumpRenderTree              0x00007bd0 main + 3884 (DumpRenderTree.m:327)
30  DumpRenderTree              0x000020c8 _start + 340 (crt.c:272)
31  DumpRenderTree              0x00001f70 start + 60

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> This patch caused regressions (DRT crashes) in LayoutTests/css1.
> 
> http://build.webkit.org/post-commit-powerpc-mac-os-x/builds/2960/step-layout-test/0

Should be fixed in r15844.

[Darin Adler]

Comment on attachment 9976 [details]
Patch to finally fix this!

Exciting to get this fixed!

I can't tell why this patch adds a concatCTM function to GraphicsContext. The rest of the patch doesn't seem to use that. Is that an independent change?

The removal of mapFromVisual and mapToVisual looks great, but it also seems independent from the rest of the patch.

The substance of the patch, in RenderSVGContainer and CSSStyleSelector, seems pretty good.

The part in adjustRenderStyle seems a little bit "literal-minded" and I also think a switch statement would be better than cascaded ifs -- for one thing we'd get a warning if we ever added new overflow values. Also, we might need to do something with our special overflow values like "overlay" and a switch statement could make that clearer.

I'm not entirely sure that the rule in requiresLayer is correct. I'd like Hyatt to review that.

Seems like this patch should add an addClip function that takes a FloatRect. I don't see a reason to land this with a FIXME -- that would be a straightforward change.

I'd like to see the unrelated cleanup landed first -- I'm tempted to just do it myself soon.

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> I'd like to see the unrelated cleanup landed first -- I'm tempted to just do it
> myself soon.

Err, this has already landed as r15842.  See Comment #14 for the review.

[Eric Seidel (no email)]

Comment on attachment 9976 [details]
Patch to finally fix this!

andersca reviewed this long ago.