JDK-8162922 : JavaFx WebView canvas doesn't support dash within strokeRec
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: windows
  • CPU: x86
  • Submitted: 2016-08-02
  • Updated: 2017-01-21
  • Resolved: 2017-01-21
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 JDK 9
8u152Fixed 9Fixed
Related Reports
Relates :  
Description
JavaFx Browser usually handles calling Context.setLineDash for any 2D canvas, but it does NOT work for when canvas is using a "strokeRect",
strokeRect.

We allow our customers to configure line dash for rectangles so it won't work right when using javafx WebView as the browser, but it works for all other browsers.

Please, refer to attached PNG image for this problem too.

This problem can be reproduced with following simple jsfiddle:

https://jsfiddle.net/8tHDf/3/

{code}
var ctx = canvas.getContext('2d');
ctx.lineWidth = 4;
ctx.setLineDash([4,4]);
ctx.strokeStyle = '#f00';
ctx.strokeRect(10,30,70,70);
{code} 
Comments
Changeset: 2ecfa257b186 Author: mbilla Date: 2017-01-21 17:15 +0530 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/2ecfa257b186
21-01-2017

+1
20-01-2017

Looks great now. +1
19-01-2017

webrev: http://cr.openjdk.java.net/~mbilla/8162922/webrev.06/
19-01-2017

"hg diff --git" patch uploaded at (showing as rename) : http://cr.openjdk.java.net/~mbilla/8162922_git_diff/StrokeRect.patch But newly generated webrev http://cr.openjdk.java.net/~mbilla/8162922/webrev.05/ still showing the file as NEW.
19-01-2017

The copyright header change is fine. The webrev does not show the renamed file as a rename, possibly because of how you generated the webrev. Please ensure that "hg diff --git" shows the file as a rename and *not* as a new file. Then, please regenerate the webrev such that it shows the file as a rename. If you generate it from uncommitted, modified files in your repo, then please also separately upload a patch generated by "hg diff --git". If you generate it from an MQ patch or from a committed changeset (which you would "rollback" after generating the webrev), then webrev will take care of it for you.
19-01-2017

webrev: http://cr.openjdk.java.net/~mbilla/8162922/webrev.04/ Corrected the order of expected and actual for assert for devicePixelRatio Note: For the renamed file (CanvasTest.java), header copyright changed from 2015 to 2015, 2017 (not sure though)
19-01-2017

webrev: http://cr.openjdk.java.net/~mbilla/8162922/webrev.03/ Incorporated above comments. I tested .03 patch for HIDPI 1.25 and with fix for JDK-8172988. The test case passed with assert for devicePixelRatio for 1. gradle -PEXTRA_TEST_ARGS='-Dglass.win.uiScale=1.25' -PCOMPILE_WEBKIT=true :web:test
19-01-2017

+ final String[] pixelArray = {"255", "0", "0", "255", + "255", "0", "0", "255", + "255", "0", "0", "255", + "255", "0", "0", "255", + "0", "0", "0", "0", + "0", "0", "0", "0", + "0", "0", "0", "0", + "0", "0", "0", "0"}; Use int array rather than String array. + private static final String htmlCanvasContent = "\n" Keep it as local variable to the testcase
19-01-2017

[~kcr] I was thinking of renaming ImageToDataURLTest.java to CanvasTest.java, since currently ImageToDataURLTest.java contains only one canvas test case and it will be good to add all canvas related test cases to this file.
19-01-2017

+1 with nit: + assertEquals("StrokeRect pixel data is same", expectedPixelArray[i-16], (int)obj.getSlot(i)); leave a space between operands and operator in an expression. e.g."i - 16" Since you are assuming pixelScale value to 1, it is better to assert. You can get devicePixel scale like below, "window.devicePixelRatio"
19-01-2017

webrev: http://cr.openjdk.java.net/~mbilla/8162922/webrev.02/ Incorporated above comments...
19-01-2017

The code change looks fine. My only comment on the new unit test (other than that it will need to wait for JDK-8172988 to be fixed) is that ImageToDataURLTest seems an odd place for it. This isn't a Data URL test. Since it doesn't share anything from the test already in ImageToDataURLTest, maybe putting it somewhere else would be cleaner?
18-01-2017

[~kcr] Thanks kevin ...
18-01-2017

I filed JDK-8172988 to track making web tests headless again.
18-01-2017

The failure on my Windows 7 machine was due to HiDPI. If I force the scale to 1 then it passes. This highlights a general (and known) problem with pixel-based tests. The :web tests should be run in headless mode, but they currently aren't. The build logic that is supposed to run the web tests in headless mode was lost in the shuffle during the recent build upgrade to building with a Jigsaw-based boot JDK. I discovered this while reviewing another test a while ago, and forgot to file the bug. I had planned to to that today anyway, so I will now do so and file it as P3 since it blocks the unit test for this bug.
18-01-2017

The unit test fails on my Windows 7 machine. It passes on Linux.
18-01-2017

webrev with unit test case: http://cr.openjdk.java.net/~mbilla/8162922/webrev.01/
18-01-2017

There is a DRT test case LayoutTests/fast/canvas/canvas-lineDash.html which tests the current issue. I executed this DRT in HelloWebView without fix and with fix and i see the number of FAIL (around 12) are same in case of without Fix and with fix. But i can see the difference in final output drawing and i attached the screenshots in both cases. One reason why the FAIL cases not reduced with the proposed fix is: I printed the pixel values both without and with fix for lineDash of 4 value and pasted below (for first 64 pixels) without FIX: 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, with FIX: 255, 0, 0, 255, 255, 0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, I compared these values in chrome and safari and observed that there is a mismatch for first pixel (with fix) for Line Dash of 4 value: First pixel with Fix: 255, 0, 0, 255, 255, 0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0 Expected first pixel: 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255, 0, 0, 255 This issue can be taken as a seperate bug. Since the DRT test case does not reflect the improvement in PASS percentage (with fix) , currently i written a unit test case to check the pixel values. The test case Passes with fix and fails without fix. I will post another webrev with unit test case.
18-01-2017

Do we have test case in DRT? Otherwise you can write a unit test(":web:test") using "ctx.getImageData" to verify pixels instead of comparing images. Refer "LayoutTests/fast/canvas/canvas-fillRect-shadow.html".
18-01-2017

webrev: http://cr.openjdk.java.net/~mbilla/8162922/webrev.00/ While Constructing BasicStroke, we also should take into account DashSize and DashOffset. This issue also can be resolved by calling BasicStroke stroke = state.getStrokeNoClone().getPlatformStroke(); instead of calling BasicStroke constructor. Tested this issue with HelloWebView in http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_canvas_tut_path with combinations of setLineDash and strokeRect like below: <script> var c = document.getElementById("myCanvas"); var ctx = c.getContext("2d"); ctx.lineWidth = 4; ctx.setLineDash([4,4]); ctx.strokeStyle = '#f00'; ctx.strokeRect(10,30,70,70); </script> Unit test case is not provided since the issue is related to rendering and unit test case has to compare images.
17-01-2017

I confirm that this happens in 9 and also in 8 GA.
02-08-2016