Bug 14015

Summary: SVG invalidates too much due to unclipped absoluteClippedOverflowRect values
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: dino, emacemac7, krit, mitz, simon.fraser, zimmermann
Priority: P2 Keywords: SVGHitList
Version: 523.x (Safari 3)   
Hardware: Macintosh   
OS: OS X 10.4   
URL: http://m8y.org/js/svgbounce2.xhtml
Bug Depends on: 23881    
Bug Blocks: 24627, 14019, 15387    
Attachments:
Description Flags
test case
none
reduced test none

Description Eric Seidel (no email) 2007-06-06 10:19:11 PDT
invalidation issue when moving SVG via js

Click "bounce", notice that the intial seal position is never re-drawn (or it's continuously redrawn in the wrong spot?)
Comment 1 Eric Seidel (no email) 2007-06-06 10:22:24 PDT
Created attachment 14881 [details]
test case

attaching test case to bug so it doesn't disappear some day
Comment 2 Alp Toker 2007-06-06 10:41:43 PDT
The top-left most corner of the SVG is invalidated at each refresh. So, the size appears to be correct, but the offset is always 0,0.
Comment 3 Eric Seidel (no email) 2007-10-06 20:22:50 PDT
Now we seem to be invalidating way too much.  This is even after my recent fixes for excess invalidation in Sun's Lively Kernel. :(
Comment 4 Eric Seidel (no email) 2007-10-07 18:35:06 PDT
Created attachment 16583 [details]
reduced test

I bet this is a bug in absoluteClippedOverflowRect.  I bet it's supposed to to actually clip the return rect to the size of the viewport (size of the <svg>).  Otherwise this is exactly the same as moving an absolutely positioned <div> around.
Comment 5 Eric Seidel (no email) 2007-10-07 18:43:57 PDT
mitz would know the correct behavior of absoluteClippedOverflowRect.  Currently RenderSVGRoot::absoluteClippedOverflowRect() returns the union of its kids bboxes.  This union is not clipped (as from the method name I would assume it should be).  I wonder though if absoluteClippedOverflowRect should ever return a rect smaller than the <svg>s bounds?
Comment 6 mitz 2007-10-08 00:34:37 PDT
An object's absoluteClippedOverflowRect should be the smallest rectangle, in absolute coordinates, containing all pixels the object paints into in its paint() method. For your typical RenderBlock it would be its overflow rect (which encompasses overhanging floats' overflow rects, because paint() paints the floats), intersected with the clip rect imposed by its ancestors, if any.

In this example, the middle (olive) div's clipped overflow rect is a 100x75 rect:

<div style="width: 100px; height: 100px; overflow: hidden;">
	<div style="width: 150px; height: 50px; background: olive;">
		<div style="width: 50px; height: 75px; background: silver;">
		</div>
	</div>
</div>

LayoutState is an optional optimization that speeds up the computation of absolute clipped rects during layout. It is arguably easier to read the non-optimized branches of the code to see how things work.
Comment 7 Eric Seidel (no email) 2007-10-09 00:24:13 PDT
I tried just fixing absoluteClippedOverflowRect() on RenderSVGRoot, but that's insufficient.  We have to fix it all the way up the chain.  What is dirtying way too much is not the root, but rather the children.  We need a way to access your parents clip rect as a child.  Possibly a clipRect() function or an applyClipToRect(rect) function which can walk up the parent chain.  We could also use/abuse the LayoutState optimization if we were careful.
Comment 8 Eric Seidel (no email) 2007-10-12 02:18:42 PDT
*** Bug 15387 has been marked as a duplicate of this bug. ***
Comment 9 Eric Seidel (no email) 2007-10-12 02:22:02 PDT
Ok, the correct fix for this is for SVG renderers to implement:
computeAbsoluteRepaintRect

And fix all of our uses (like in SVGRenderImage) of RenderObject::repaintRect to use renderer-relative offsets instead of absolute offsets!

implementing computeAbsoluteRepaintRect correctly will also fix our partial-repaint issues with text:

void RenderSVGText::layout()
{
    ASSERT(needsLayout());
    
    // FIXME: This is a hack to avoid the RenderBlock::layout() partial repainting code which is not (yet) SVG aware
    setNeedsLayout(true);


It might also fix our repaint issues with text when switching from display:none to display: inline and back, not certain on that last one.
Comment 10 Eric Seidel (no email) 2007-12-15 04:06:51 PST
This is by far the worst bug affecting the Sun Labs Lively Kernel.
Comment 11 Eric Seidel (no email) 2009-10-07 13:34:29 PDT
The original bug seems much better, probably from the rendering tree refactor this spring.  We're still invalidating too much for the lively kernel, but I think we need better test cases to show that.
Comment 12 Nikolas Zimmermann 2010-07-08 23:56:42 PDT

*** This bug has been marked as a duplicate of bug 41854 ***