RESOLVED FIXED 33406
Repaint bug dragging a star shape with a dash stroke
https://bugs.webkit.org/show_bug.cgi?id=33406
Summary Repaint bug dragging a star shape with a dash stroke
Beth Dakin
Reported 2010-01-08 15:52:04 PST
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.
Attachments
Test case (2.43 KB, text/html)
2010-01-08 15:52 PST, Beth Dakin
no flags
fix of strokeBoundingBox in RenderPath (845 bytes, patch)
2010-01-09 03:45 PST, Dirk Schulze
no flags
fix of strokeBoundingBox in RenderPath (19.39 KB, patch)
2010-01-13 01:13 PST, Dirk Schulze
no flags
fix of strokeBoundingBox in RenderPath (144.77 KB, patch)
2010-01-13 10:52 PST, Dirk Schulze
bdakin: review+
commit-queue: commit-queue-
forgot two results in previous patch (5.09 KB, patch)
2010-01-13 14:32 PST, Dirk Schulze
bdakin: review+
Dirk Schulze
Comment 1 2010-01-09 03:40:22 PST
(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
Comment 2 2010-01-09 03:45:46 PST
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
Comment 3 2010-01-11 11:46:03 PST
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
Comment 4 2010-01-11 11:47:22 PST
(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
Comment 5 2010-01-11 13:30:04 PST
> 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
Comment 6 2010-01-11 13:34:03 PST
(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
Comment 7 2010-01-13 01:13:39 PST
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
Comment 8 2010-01-13 10:52:27 PST
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 9 2010-01-13 11:01:46 PST
Comment on attachment 46475 [details] fix of strokeBoundingBox in RenderPath r=me
Dirk Schulze
Comment 10 2010-01-13 11:36:55 PST
Comment on attachment 46475 [details] fix of strokeBoundingBox in RenderPath Thanks for the quick review :-)
WebKit Commit Bot
Comment 11 2010-01-13 11:54:13 PST
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
Comment 12 2010-01-13 13:15:33 PST
Updating the failing tests. Will upload a second patch that fixes the missing result updates.
Dirk Schulze
Comment 13 2010-01-13 14:32:01 PST
Created attachment 46510 [details] forgot two results in previous patch forgot two results in previous patch.
Dirk Schulze
Comment 14 2010-01-13 15:01:31 PST
landed in r53207. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.