Bug 82963 - REGRESSION (r101517): Animating the transform of a <rect> with shape-rendering: crispEdges leaves behind garbage
Summary: REGRESSION (r101517): Animating the transform of a <rect> with shape-renderin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-04-02 16:04 PDT by Tim Horton
Modified: 2012-10-25 11:50 PDT (History)
5 users (show)

See Also:


Attachments
repro (433 bytes, image/svg+xml)
2012-04-02 16:04 PDT, Tim Horton
no flags Details
screenshot (8.56 KB, image/png)
2012-04-02 16:05 PDT, Tim Horton
no flags Details
manual testcase (676 bytes, image/svg+xml)
2012-04-02 19:33 PDT, Tim Horton
no flags Details
patch sans test (6.54 KB, patch)
2012-04-02 20:17 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff
Scaled test (473 bytes, image/svg+xml)
2012-10-25 11:50 PDT, Florin Malita
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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