strokeRect behaves inconsistently and differently from the equivalent rect(), stroke() sequence. In the linked fiddle a rectangle stroke is requested with a lineJoin of 'bevel'. strokeRect incorrectly ignores it when the fill style is 'black', but applies it correctly if the fill style is a gradient. A rect(), stroke() sequence works correctly. The test works fine on Chrome on any platform, as well as Firefox and Opera on Linux. It fails on Safari MacOS and iOS (tested on MacOS 10.9.2, version 7.0.3, as well as iOS 7.1.1).
Created attachment 231503 [details] Safari rendering
Created attachment 231504 [details] Expected rendering
Created attachment 235411 [details] Patch
Comment on attachment 235411 [details] Patch Attachment 235411 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6136317259808768 New failing tests: fast/canvas/canvas-strokeRect-alpha-shadow.html canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235418 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235411 [details] Patch Attachment 235411 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4969975940907008 New failing tests: fast/canvas/canvas-strokeRect-alpha-shadow.html canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235423 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235411 [details] Patch Attachment 235411 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6083132679782400 New failing tests: media/track/add-and-remove-track.html canvas/philip/tests/2d.strokeRect.zero.5.html fast/canvas/canvas-strokeRect-alpha-shadow.html
Created attachment 235427 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 235498 [details] Patch
Comment on attachment 235498 [details] Patch Attachment 235498 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5723360449265664 New failing tests: canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235501 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 235498 [details] Patch Attachment 235498 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5204476860301312 New failing tests: canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235504 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235498 [details] Patch Attachment 235498 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6059106632728576 New failing tests: canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235505 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235498&action=review Thanks for tackling this. Test case is failing, can’t land this until that’s resolved. > Source/WebCore/ChangeLog:3 > + [CANVAS] strokeRect does not honor lineJoin Bug naming is wrong here. We don’t include "[CANVAS]" in the title of a bug just because it's a canvas bug fix. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1170 > + // Using CGContextAddRect and CGContextStrokePath to stroke rect rather than > + // convenience functions (CGContextStrokeRect and CGContextStrokeRectWithWidth). > + // The convenience functions fail to apply some attributes of the graphics state > + // in certain cases (e.g. See https://bugs.webkit.org/show_bug.cgi?id=132948) > + CGContextAddRect(context, rect); > + CGContextStrokePath(context); The comment says that there is a bug in CGContextStrokeRect and CGContextStrokeRectWithWidth “in certain cases” and provides a link to this bug. But nothing in this bug documents the problem with these functions, nor do we have a report to the team at Apple that handles CG describing the bug. I’d like to be clearer on what the bug is before we work around it. The fact that the tests are failing also makes me suspect that we don’t yet know the whole story. This code as is cannot be correct, because it ignores the passed-in line width. We at least need CGContextSaveGState, setStrokeThickness, and CGContextRestoreGState calls in addition to the calls here, as in the non-shadow gradient code path above. To test this, I presume we need test cases with different line widths and we should see failures unless we set the line width. It’s possible this breaks the use of strokeRect in RenderEmbeddedObject and RenderSVGRect, so we may need test cases that cover those uses too.
Created attachment 235667 [details] Patch
(In reply to comment #17) > (From update of attachment 235498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235498&action=review > > Thanks for tackling this. Thanks for the review. > > Test case is failing, can’t land this until that’s resolved. > I have tried running this test on a number of machines over here (including on OSX 10.9.4 and 10.8.5) and I cant seem to reproduce the failure. Do you know any other way that I can reproduce this issue? > > Source/WebCore/ChangeLog:3 > > + [CANVAS] strokeRect does not honor lineJoin > > Bug naming is wrong here. We don’t include "[CANVAS]" in the title of a bug just because it's a canvas bug fix. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1170 > > + // Using CGContextAddRect and CGContextStrokePath to stroke rect rather than > > + // convenience functions (CGContextStrokeRect and CGContextStrokeRectWithWidth). > > + // The convenience functions fail to apply some attributes of the graphics state > > + // in certain cases (e.g. See https://bugs.webkit.org/show_bug.cgi?id=132948) > > + CGContextAddRect(context, rect); > > + CGContextStrokePath(context); > > The comment says that there is a bug in CGContextStrokeRect and CGContextStrokeRectWithWidth “in certain cases” and provides a link to this bug. But nothing in this bug documents the problem with these functions, nor do we have a report to the team at Apple that handles CG describing the bug. I’d like to be clearer on what the bug is before we work around it. The fact that the tests are failing also makes me suspect that we don’t yet know the whole story. Until I can reproduce the failure thats occurring on the mac bot I don't think I can be totally sure of the cause of the bug. However, the patch does also seem to address previous issues with the fast/canvas/canvas-strokeRect-alpha-shadow.html test (that is, the results are now inline with the general expected results instead of the overriden results under LayoutTests/platform/mac/). > > This code as is cannot be correct, because it ignores the passed-in line width. We at least need CGContextSaveGState, setStrokeThickness, and CGContextRestoreGState calls in addition to the calls here, as in the non-shadow gradient code path above. > > To test this, I presume we need test cases with different line widths and we should see failures unless we set the line width. It’s possible this breaks the use of strokeRect in RenderEmbeddedObject and RenderSVGRect, so we may need test cases that cover those uses too. Agreed (this was an oversight) - I've updated the latest patch with these changes.
Comment on attachment 235667 [details] Patch Attachment 235667 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5401232533356544 New failing tests: canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235668 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235667 [details] Patch Attachment 235667 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5800967287078912 New failing tests: canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235673 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 235667 [details] Patch Attachment 235667 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6655030123823104 New failing tests: media/track/add-and-remove-track.html canvas/philip/tests/2d.strokeRect.zero.5.html
Created attachment 235678 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 236008 [details] Patch
Created attachment 236009 [details] TestCGContextStrokeRect test Attachment TestCGContextStrokeRect is a standalone test that reproduces the context graphics state in the WebKit code that causes the CGContextStrokeRectWithWidth convenience function to fail (i.e. the combination of scaling, using bevel line join with the IOSurface context)
Created attachment 236010 [details] TestCGContextStrokeRect_Output.png TestCGContextStrokeRect_Output.png shows the output from the TestCGContextStrokeRect test. The rectangle on the left incorrectly does not have the bevel line join applied.
Created attachment 236011 [details] TestStrokeRectZero test Attachment TestStrokeRectZero is a standalone test that reproduces the test case canvas/philip/tests/2d.strokeRect.zero.5.html (plus an addition test case) that shows the difference in output from OSX 10.8.5 and 10.9.4. 10.8.5 seems to show the incorrect result when using the CGContextAddRect/CGContextStrokePath combination compared to the convenience function. OSX 10.9.4 is rendering the test case correctly.
Created attachment 236012 [details] TestStrokeRectZero_ExampleOutput_OSX_10_8_5.png Shows example output from the TestStrokeRectZero standalone test (Test case 2) from OSX 10.8.5
Created attachment 236013 [details] TestStrokeRectZero_ExampleOutput_OSX_10_9_4.png Shows example output from the TestStrokeRectZero standalone test (Test case 2) from OSX 10.9.4
I have some more information on the bug and associated patch: > The comment says that there is a bug in CGContextStrokeRect and CGContextStrokeRectWithWidth “in certain cases” and provides a link to this bug. But nothing in this bug documents the problem with these functions, nor do we have a report to the team at Apple that handles CG describing the bug. I’d like to be clearer on what the bug is before we work around it. The fact that the tests are failing also makes me suspect that we don’t yet know the whole story. I've spent some time isolating the bug in CG. The attached test case TestCGContextStrokeRect.tar reproduces the context graphics state causing the failure and shows that the convenience function CGContextStrokeRectWithWidth does contain a bug (see TestCGContextStrokeRect_Output.png - the left rectangle should have a bevel line join applied). Could you bring this to the CG teams attention? Or let me know how I can? Regarding the failing test case canvas/philip/tests/2d.strokeRect.zero.5.html for this patch - this looks like it has been fixed in OSX Mavericks (see the TestStrokeRectZero test case and associated output) Additionally, the patch corrects the previously failing results from the fast/canvas/canvas-strokeRect-alpha-shadow.html test case. Therefore, with the current state of the convenience function versus the CGContextAddRect/CGContextStrokePath combination functionality - I think that the patch is a valid solution (at least until the convenience functions are fixed).
(In reply to comment #32) > I've spent some time isolating the bug in CG. The attached test case TestCGContextStrokeRect.tar reproduces the context graphics state causing the failure and shows that the convenience function CGContextStrokeRectWithWidth does contain a bug (see TestCGContextStrokeRect_Output.png - the left rectangle should have a bevel line join applied). > Could you bring this to the CG teams attention? Or let me know how I can? You can report a Core Graphics bug at <http://bugreport.apple.com>. It requires signing up for a (free) developer account, but it's pretty straightforward.
(In reply to comment #33) > (In reply to comment #32) > > I've spent some time isolating the bug in CG. The attached test case TestCGContextStrokeRect.tar reproduces the context graphics state causing the failure and shows that the convenience function CGContextStrokeRectWithWidth does contain a bug (see TestCGContextStrokeRect_Output.png - the left rectangle should have a bevel line join applied). > > Could you bring this to the CG teams attention? Or let me know how I can? > > You can report a Core Graphics bug at <http://bugreport.apple.com>. It requires signing up for a (free) developer account, but it's pretty straightforward. I have filed a bug on Core Graphics through that link. The id is 17925995. With the title: [CG] CGContextStrokeRectWithWidth ignores some graphics state settings In the meantime, should we land this WK patch?
Comment on attachment 236008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236008&action=review I imagine this is measurably slower than using CGContextStrokeRectWithWidth, so we would like to be on a path where some day we go back to using that function. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1167 > + // The convenience functions currently (in at least OSX 10.9.4) fail to “OSX” is incorrect; “OS X” is correct
Comment on attachment 236008 [details] Patch Clearing flags on attachment: 236008 Committed r172119: <http://trac.webkit.org/changeset/172119>
All reviewed patches have been landed. Closing bug.