Bug 33406 – Repaint bug dragging a star shape with a dash stroke

• Component: SVG
• Status: RESOLVED FIXED

[Beth Dakin]

Created attachment 46170 [details]
Test case

This bug was discovered while investigating https://bugs.webkit.org/show_bug.cgi?id=32757

Open the test case and try to drag the third star. Note that small grey dots are left behind as the star is dragged. I suspect that there may be a problem where strokeBoundingBox() is not quite big enough in this odd case where the right-most point of the shape is a blank spot in the dash stroke. Of course, I could be wrong too :-)

Also, please note that the test case of course is manual, but if moved into the fast/repaint directory, it will be an automated test too.

[Dirk Schulze]

(In reply to comment #0)
> Created an attachment (id=46170) [details]
> Test case
> 
> This bug was discovered while investigating
> https://bugs.webkit.org/show_bug.cgi?id=32757
> 
> Open the test case and try to drag the third star. Note that small grey dots
> are left behind as the star is dragged. I suspect that there may be a problem
> where strokeBoundingBox() is not quite big enough in this odd case where the
> right-most point of the shape is a blank spot in the dash stroke. Of course, I
> could be wrong too :-)
> 
> Also, please note that the test case of course is manual, but if moved into the
> fast/repaint directory, it will be an automated test too.

Yes, the problem was in strokeBoundingBox. The strokeRect must not contain the boundingBox of the path. We need the union of both.

[Dirk Schulze]

Created attachment 46204 [details]
fix of strokeBoundingBox in RenderPath

This is the fix for the problem. I'm not sure how to make a regression test for this bug. I think it just needs a copy of Beth's shadow repaint test without shadows :-D

[Beth Dakin]

Dirk, the patch looks great! The test case that I attached to this bug is actually already  layout test, but only it you add it to the fast/repaint directory since it relies on some javascript that is stored in fast/repaint/resources. If you add this test case there and attach a new patch, I will r+.

Also, the automated version of the test case that I attached is pretty subtle; it just leaves behind two pixels of grey that shouldn't be there. I don't think there is a way to make it more obvious with our current repaint test mechanism, but if this is something that concerns you, you could change the color of the stars to something that will be more obvious, like black instead of grey, and you could always add some text to the test about how it works. None of this is really necessary though.

[Beth Dakin]

(In reply to comment #3)
>The test case that I attached to this bug is
> actually already  layout test, but only it you add it to the fast/repaint

This should say, "The test case that I attached to this bug is actually already a layout test, but only IF you add it to the fast/repaint…"

[Alexey Proskuryakov]

> it just leaves behind two pixels of grey that shouldn't be there

Does that mean that the test is ineffective due to to run-webkit-tests tolerating small differences by default?

[Beth Dakin]

(In reply to comment #5)
> > it just leaves behind two pixels of grey that shouldn't be there
> 
> Does that mean that the test is ineffective due to to run-webkit-tests
> tolerating small differences by default?

Good question. I haven't actually run it through DumpRenderTree, so I am not sure.

[Dirk Schulze]

Created attachment 46434 [details]
fix of strokeBoundingBox in RenderPath

Same patch with simple test of the repaint calculation. I'm not sure if other tests are involved by this patch. My Mac Os system is currently broken. Could some one run webkit-tests with this patch please?

[Dirk Schulze]

Created attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Got my system running again. This is the same patch as above. I was surprised how many tests are affected by this changes. I checked the results and the repaint rect got always bigger, and are correct according to the tests output.

[Beth Dakin]

Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

r=me

[Dirk Schulze]

Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Thanks for the quick review :-)

[WebKit Commit Bot]

Comment on attachment 46475 [details]
fix of strokeBoundingBox in RenderPath

Rejecting patch 46475 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11919 test cases.
fast/repaint/moving-shadow-on-container.html -> failed

Exiting early after 1 failures. 7967 tests run.
129.29s total testing time

7966 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/184578

[Dirk Schulze]

Updating the failing tests. Will upload a second patch that fixes the missing result updates.

[Dirk Schulze]

Created attachment 46510 [details]
forgot two results in previous patch

forgot two results in previous patch.

[Dirk Schulze]

landed in r53207. Closing bug.