Bug 82963

Summary: REGRESSION (r101517): Animating the transform of a <rect> with shape-rendering: crispEdges leaves behind garbage
Product: WebKit Reporter: Tim Horton <thorton>
Component: SVGAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: fmalita, rhodovan.u-szeged, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
repro
none
screenshot
none
manual testcase
none
patch sans test
simon.fraser: review+
Scaled test none

Description Tim Horton 2012-04-02 16:04:23 PDT
Created attachment 135214 [details]
repro

Like the title says, animating the transform of a <rect> with shape-rendering: crispEdges leaves behind garbage. Probably the repaint rect is rounded to be too small by 1px or something.

See the attached test case. I ran into this with a very simple d3.js toy I was writing over the weekend, so I'm pretty sure it can adversely affect real world content. Bisection says 101517 is probably the culprit (makes sense to me!).
Comment 1 Radar WebKit Bug Importer 2012-04-02 16:05:00 PDT
<rdar://problem/11170476>
Comment 2 Tim Horton 2012-04-02 16:05:11 PDT
Created attachment 135215 [details]
screenshot
Comment 3 Tim Horton 2012-04-02 18:39:18 PDT
Inflating the outer stroke rect (which is returned for strokeBoundingBox) of the RenderSVGRect by 1px fixes this.

It looks like CG can draw an extra 1px part of the stroke when drawing a rectangle with AA off, but not a path-made-from-a-rectangle, which is why we didn't run into this before 101517.

I have a patch to fix, pending a good test.
Comment 4 Tim Horton 2012-04-02 19:33:42 PDT
Created attachment 135261 [details]
manual testcase

I can't make it reproduce in a test, something always causes extra repaint! (which happens if you click away from the browser, or scroll, or just wait a minute, or whatever, so it's not a very stable test to begin with).
Comment 5 Tim Horton 2012-04-02 20:17:02 PDT
Created attachment 135266 [details]
patch sans test
Comment 6 Tim Horton 2012-04-06 14:06:57 PDT
I cannot make this reproduce in WebKit1 at all (even in Safari). I can't make it reproduce in DRT or WKTR at all.

Simon, any qualms about checking in without a test? There's already a comment there which will hopefully dissuade anyone from removing the code.
Comment 7 Simon Fraser (smfr) 2012-04-06 14:17:44 PDT
Nah, that's OK
Comment 8 Tim Horton 2012-04-06 14:31:44 PDT
Landed in http://trac.webkit.org/changeset/113496
Comment 9 Florin Malita 2012-10-25 11:49:52 PDT
I noticed this patch while looking into a somewhat similar Skia problem: https://bugs.webkit.org/show_bug.cgi?id=100374

m_strokeBoundingRect.inflate(1);

^^ this doesn't look right: we are not in device space here, and the above may or may not inflate the absolute repaint rect by one pixel (see the new test with a scale applied).

Since both CG and Skia seem to need repaint rect adjustments, maybe we can add a platform call for this purpose. But it's unclear what the best place for such an adjustment is: on one hand it makes sense to do it at the lowest level, where we know the stroke properties; on the other hand it's tricky to make screen space decisions here - but then the higher levels are clueless about a possible stroke on the edge of the repaint rect... Ideas?
Comment 10 Florin Malita 2012-10-25 11:50:29 PDT
Created attachment 170701 [details]
Scaled test