Re: RFR: 8280020: Underline and line-through not straight in WebView [v8]
On Mon, 7 Mar 2022 09:28:52 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > formating for drawline Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v8]
On Mon, 7 Mar 2022 09:28:52 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > formating for drawline Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v8]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: formating for drawline - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/355c798b..e7d39427 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=06-07 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v7]
On Fri, 4 Mar 2022 03:29:33 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with two additional > commits since the last revision: > > - test needs width and height , during creattion of webview > - test needs width and height , during creattion of webview Looks great. I confirm that the updated test fails without the fix and passes with the fix on both macOS and Windows. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v7]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with two additional commits since the last revision: - test needs width and height , during creattion of webview - test needs width and height , during creattion of webview - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/5ab81018..355c798b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=05-06 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Mon, 21 Feb 2022 02:41:13 GMT, Jay Bhaskar wrote: >> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line >> 75: >> >>> 73: this.primaryStage = primaryStage; >>> 74: this.primaryStage.setWidth(80); >>> 75: this.primaryStage.setHeight(60); >> >> Minor: Since you set the size of the Scene later on, you don't need to set >> it here. > > This would be remain same , i would not set size later It looks like you lost the earlier changes we had discussed. You should not set the size here (and certainly not to this small of a value). - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v6]
On Tue, 1 Mar 2022 03:24:57 GMT, Jay Bhaskar wrote:
>> Issue: The end point of line in drawLinesForText , add thickness to the
>> endPoint.y(). In this case origin which is start point and the end point
>> would not be same, and line would be drawn not straight.
>> Solution: Do not add thickness to the y position of end point of line.
>> Start Point(x,y) --End Point(x + width, 0)
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> PR review changes applied
The test fails for me on both Mac and Windows because the scene is too small.
It looks like the earlier changes you had made (or were in the process of
making) got lost. See below.
When I eliminate the setting of the size on the stage and construct the scene
with a size of `150,100` it works on both platforms.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 108:
> 106: Platform.runLater(() -> {
> 107: webView = new WebView();
> 108: Scene scene = new Scene(webView);
It looks like you lost the earlier changes we had discussed. You should set the
size here to _at least_ `150,100`.
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v6]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: PR review changes applied - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/29916450..5ab81018 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=04-05 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
On Mon, 28 Feb 2022 15:12:37 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> New chnages for line test
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 168:
>
>> 166: int width =
>> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().width");
>> 167:
>> 168: int line_start_x = start_x;
>
> You might want to add a small amount (`DELTA`?) to avoid sampling at the edge.
In test html , the margin and padding is 0 px. it would be ok to add DELTA, done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 173:
>
>> 171: String line_color = "rgba(0,0,0,255)"; // color of line
>> 172: System.out.println(line_end_x);
>> 173: System.out.println(line_start_y);
>
> Once you are done debugging the test, you can remove these print statements.
ok,
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 177:
>
>> 175: for (int x = line_start_x; x < line_end_x; x++) {
>> 176: String color = colorToString(pr.getColor(x,
>> line_start_y));
>> 177: assertEquals(color, line_color);
>
> The arguments are backwards (the expected value goes first).
done
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
On Mon, 28 Feb 2022 15:10:27 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> New chnages for line test > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 57: > >> 55: private static final CountDownLatch launchLatch = new >> CountDownLatch(1); >> 56: private static final int LINE_THICKNESS = 20; >> 57: private static final int SKIP_TEXT_BOUNDARY = 32; > > What does this constant represent? LINE_THICKNESS is thickness that is also in html test code. SKIP_TEXT_BOUNDARY is used to skip boundary at the end of line. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
On Sun, 27 Feb 2022 07:10:25 GMT, Jay Bhaskar wrote:
>> Issue: The end point of line in drawLinesForText , add thickness to the
>> endPoint.y(). In this case origin which is start point and the end point
>> would not be same, and line would be drawn not straight.
>> Solution: Do not add thickness to the y position of end point of line.
>> Start Point(x,y) --End Point(x + width, 0)
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> New chnages for line test
The cleaned up test looks much better. I'll test it on Mac and Windows. I left
a few additional comments.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 57:
> 55: private static final CountDownLatch launchLatch = new
> CountDownLatch(1);
> 56: private static final int LINE_THICKNESS = 20;
> 57: private static final int SKIP_TEXT_BOUNDARY = 32;
What does this constant represent?
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 166:
> 164: int start_y =
> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().y");
> 165: int height =
> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().height");
> 166: int width =
> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().width");
Minor: I recommend switching `width` and `height` to match the order of `x` and
`y`.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 168:
> 166: int width =
> (int)webView.getEngine().executeScript("document.getElementsByTagName('div')[0].getBoundingClientRect().width");
> 167:
> 168: int line_start_x = start_x;
You might want to add a small amount (`DELTA`?) to avoid sampling at the edge.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 173:
> 171: String line_color = "rgba(0,0,0,255)"; // color of line
> 172: System.out.println(line_end_x);
> 173: System.out.println(line_start_y);
Once you are done debugging the test, you can remove these print statements.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 177:
> 175: for (int x = line_start_x; x < line_end_x; x++) {
> 176: String color = colorToString(pr.getColor(x,
> line_start_y));
> 177: assertEquals(color, line_color);
The arguments are backwards (the expected value goes first).
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: New chnages for line test - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/3246375d..29916450 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=03-04 Stats: 30 lines in 1 file changed: 9 ins; 7 del; 14 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v4]
On Mon, 21 Feb 2022 06:02:32 GMT, Jay Bhaskar wrote:
>> Issue: The end point of line in drawLinesForText , add thickness to the
>> endPoint.y(). In this case origin which is start point and the end point
>> would not be same, and line would be drawn not straight.
>> Solution: Do not add thickness to the y position of end point of line.
>> Start Point(x,y) --End Point(x + width, 0)
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve style and width iterartion logic
I added one more minor formatting comment. The important points are listed as
replies to your earlier comments above: The size of the window needs to be
larger, and the sampling logic isn't quite right.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 172:
> 170: continue;
> 171: }
> 172: else {
Minor: this should be on one line, like this:
} else {
Alternatively, you can replace the entire if-then-else block with:
assertEquals("Pixel color does not match", expected_line_color,
actual_line_color);
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Mon, 21 Feb 2022 01:57:29 GMT, Jay Bhaskar wrote:
>> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
>> 105:
>>
>>> 103: Platform.runLater(() -> {
>>> 104: webView = new WebView();
>>> 105: Scene scene = new Scene(webView,80,60);
>>
>> This test passes on my Windows system even without the fix. Based on what I
>> see on the screen, I think its because the scene size is too small. I would
>> make it larger (at least 150x100).
>>
>> Minor: add a space after the commas to separate the args
>
> space added, i will look this observation on windows
I can confirm that even on Mac, the only reason that this test passes -- and it
does so either with or without the fix -- is because the window size is too
small. You need to make the Scene larger. I recommend this:
new Scene(webView, 200, 150);
It's generally better to set the size of the scene rather than the stage for
tests like this -- especially since you are taking a snapshot of the scene.
>> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
>> 167:
>>
>>> 165: int line_start_y = start_y + height;
>>> 166: String line_color = "rgba(0,0,0,255)"; // color of line
>>> 167: for (int i = line_start_y; i < snapshot.getHeight(); i++) {
>>
>> The snapshot height is irrelevant. You should use thickness minus 6 (3
>> pixels on each of the top and bottom):
>>
>>
>> i < line_start_y + thickness - 6;
>
> I think it should be likefor (int i = line_start_y; i <= (width -
> line_start_y -6); i++) , as we need to iterate in x direction
No, see my earlier comment. it needs to use thickness (which is the height of
the line) not width.
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Mon, 21 Feb 2022 05:30:36 GMT, Jay Bhaskar wrote:
>> The bounding rect.x rect.y is top left corner, and line is being drawn below
>> the bottom, so height and thickness need to be considered.
>
> for (int i = line_start_y; i <= (width - line_start_y -6); i++) { , this
> meets the condition of sampling near the right edge
You are mixing values that should affect `x` (width) with values that should
affect `y` (height and thickness). Using height or thickness to adjust the `x`
value or using width to adjust the `y` value is wrong.
One thing that might help make it clearer is to use `y` as the name of the loop
variable rather than `i`. Another thing that might help is to define a
`thickness` constant rather than using 20. Then it will be more obvious where
you are mixing x and y.
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v4]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Improve style and width iterartion logic - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/135a073b..3246375d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=02-03 Stats: 13 lines in 1 file changed: 4 ins; 1 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 15:03:38 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 167:
>
>> 165: int line_start_y = start_y + height;
>> 166: String line_color = "rgba(0,0,0,255)"; // color of line
>> 167: for (int i = line_start_y; i < snapshot.getHeight(); i++) {
>
> The snapshot height is irrelevant. You should use thickness minus 6 (3 pixels
> on each of the top and bottom):
>
>
> i < line_start_y + thickness - 6;
I think it should be likefor (int i = line_start_y; i <= (width -
line_start_y -6); i++) , as we need to iterate in x direction
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Mon, 21 Feb 2022 04:19:29 GMT, Jay Bhaskar wrote: >> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line >> 163: >> >>> 161: >>> 162: // buttom start x position of underline ( startx + font >>> size + thickness -1) >>> 163: int line_start_x = start_x + height + 20 - 1; >> >> The x value shouldn't be affected by thickness or the height. This should be >> something like `start_x + 3` (the `+3` is so you don't sample anywhere near >> the edge, to avoid boundary problems). >> >> I also recommend sampling near the right edge to catch the problem of a >> slanted line, so: `int line_end_x = start_x + width - 3;` > > The bounding rect.x rect.y is top left corner, and line is being drawn below > the bottom, so height and thickness need to be considered. int i = line_start_y; i < (line_start_y - 6); i++ , this meets the condition of sampling near the right edge - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:55:30 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update test case for line straightness check > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 163: > >> 161: >> 162: // buttom start x position of underline ( startx + font >> size + thickness -1) >> 163: int line_start_x = start_x + height + 20 - 1; > > The x value shouldn't be affected by thickness or the height. This should be > something like `start_x + 3` (the `+3` is so you don't sample anywhere near > the edge, to avoid boundary problems). > > I also recommend sampling near the right edge to catch the problem of a > slanted line, so: `int line_end_x = start_x + width - 3;` The bounding rect.x rect.y is top left corner, and line is being drawn below the bottom, so height and thickness need to be considered. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:45:53 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update test case for line straightness check > > tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line > 75: > >> 73: this.primaryStage = primaryStage; >> 74: this.primaryStage.setWidth(80); >> 75: this.primaryStage.setHeight(60); > > Minor: Since you set the size of the Scene later on, you don't need to set it > here. This would be remain same , i would not set size later - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 16:51:52 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 172:
>
>> 170: continue;
>> 171: else
>> 172: fail("Each pixel color of line should be" +
>> line_color + " but was:" + expected);
>
> The name `expected` is misleading. It isn't the expected value, but rather
> the "actual" value that you just read.
>
> Also, there are two formatting issues:
> 1. There should be a space after the `if`
> 2. The target statements of the `if` and `else` must be surrounded by curly
> braces (unless on the same line as the `if` which would look odd in this case)
done
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 14:32:46 GMT, Kevin Rushforth wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update test case for line straightness check
>
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 78:
>
>> 76: launchLatch.countDown();
>> 77: }
>> 78: }
>
> Add a blank after this.
done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 87:
>
>> 85: }
>> 86:
>> 87:
>
> Minor: the extra blank line is unnecessary.
done
> tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line
> 105:
>
>> 103: Platform.runLater(() -> {
>> 104: webView = new WebView();
>> 105: Scene scene = new Scene(webView,80,60);
>
> This test passes on my Windows system even without the fix. Based on what I
> see on the screen, I think its because the scene size is too small. I would
> make it larger (at least 150x100).
>
> Minor: add a space after the commas to separate the args
space added, i will look this observation on windows
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
On Sat, 19 Feb 2022 03:52:41 GMT, Jay Bhaskar wrote:
>> Issue: The end point of line in drawLinesForText , add thickness to the
>> endPoint.y(). In this case origin which is start point and the end point
>> would not be same, and line would be drawn not straight.
>> Solution: Do not add thickness to the y position of end point of line.
>> Start Point(x,y) --End Point(x + width, 0)
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update test case for line straightness check
The test is a good start, but I left several comments inline.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 75:
> 73: this.primaryStage = primaryStage;
> 74: this.primaryStage.setWidth(80);
> 75: this.primaryStage.setHeight(60);
Minor: Since you set the size of the Scene later on, you don't need to set it
here.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 78:
> 76: launchLatch.countDown();
> 77: }
> 78: }
Add a blank after this.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 87:
> 85: }
> 86:
> 87:
Minor: the extra blank line is unnecessary.
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 105:
> 103: Platform.runLater(() -> {
> 104: webView = new WebView();
> 105: Scene scene = new Scene(webView,80,60);
This test passes on my Windows system even without the fix. Based on what I see
on the screen, I think its because the scene size is too small. I would make it
larger (at least 150x100).
Minor: add a space after the commas to separate the args
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 163:
> 161:
> 162: // buttom start x position of underline ( startx + font size
> + thickness -1)
> 163: int line_start_x = start_x + height + 20 - 1;
The x value shouldn't be affected by thickness or the height. This should be
something like `start_x + 3` (the `+3` is so you don't sample anywhere near the
edge, to avoid boundary problems).
I also recommend sampling near the right edge to catch the problem of a slanted
line, so: `int line_end_x = start_x + width - 3;`
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 165:
> 163: int line_start_x = start_x + height + 20 - 1;
> 164: // buttom start y position of underline ( startx + height)
> 165: int line_start_y = start_y + height;
As mentioned above, I recommend adding 3 pixels to avoid sampling near the top
edge of the thick line, so `start_y + height + 3`
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 167:
> 165: int line_start_y = start_y + height;
> 166: String line_color = "rgba(0,0,0,255)"; // color of line
> 167: for (int i = line_start_y; i < snapshot.getHeight(); i++) {
The snapshot height is irrelevant. You should use thickness minus 6 (3 pixels
on each of the top and bottom):
i < line_start_y + thickness - 6;
tests/system/src/test/java/test/javafx/scene/web/StraightLineTest.java line 172:
> 170: continue;
> 171: else
> 172: fail("Each pixel color of line should be" +
> line_color + " but was:" + expected);
The name `expected` is misleading. It isn't the expected value, but rather the
"actual" value that you just read.
Also, there are two formatting issues:
1. There should be a space after the `if`
2. The target statements of the `if` and `else` must be surrounded by curly
braces (unless on the same line as the `if` which would look odd in this case)
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
On Sun, 13 Feb 2022 07:35:32 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > adjust thickness A new system test case for checking line straightness added - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v3]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Update test case for line straightness check - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/c5b6ff76..135a073b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=01-02 Stats: 238 lines in 2 files changed: 176 ins; 62 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
On Sun, 13 Feb 2022 07:35:32 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > adjust thickness Good. So the only problem with the dashed case was the fact that we were looping and drawing the same dashed line _N_ times. Do you know whether my guess about why we need to add `thickness/2` to `y` (caller is passing in the upper-left corner) is right? In addition to a little more testing, I think we should do the following: 1. Edit the title of the JBS bug and this PR to indicate the somewhat broadened scope of the bug. Something like "WebView text underline and line-through rendered incorrectly" 2. Create a new system test that will render a thick solid line and a thick dashed line, use snapshot to capture the rendered output, and check that the line is drawn in the right place. If the thickness is large enough (at least 6 pixels) you can sample 4 pixels vertically near the center of where the line should be and it should be black. For the dashed line, you can do the same in the middle (horizontally) of both an "on" segment, which should be black, and an "off" segment, which should be white. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
On Sun, 13 Feb 2022 07:35:32 GMT, Jay Bhaskar wrote: >> Issue: The end point of line in drawLinesForText , add thickness to the >> endPoint.y(). In this case origin which is start point and the end point >> would not be same, and line would be drawn not straight. >> Solution: Do not add thickness to the y position of end point of line. >> Start Point(x,y) --End Point(x + width, 0) > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > adjust thickness The dash case (--) also works without looping https://user-images.githubusercontent.com/6468850/153743964-647e00cd-1dab-4ffb-9686-8a04c5844b98.png;> - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView [v2]
> Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: adjust thickness - Changes: - all: https://git.openjdk.java.net/jfx/pull/731/files - new: https://git.openjdk.java.net/jfx/pull/731/files/94a493ae..c5b6ff76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=731=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=731=00-01 Stats: 15 lines in 1 file changed: 2 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jfx/pull/731.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/731/head:pull/731 PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) StrokeStyle savedStrokeStyle = strokeStyle(); setStrokeStyle(stroke); float savedStrokeThickness = strokeThickness(); setStrokeThickness(thickness); FloatPoint startPoint = origin + FloatPoint(0, thickness / 2); FloatPoint endPoint = startPoint + FloatPoint(width, 0); drawLine( IntPoint(startPoint.x(), startPoint.y()), IntPoint(endPoint.x(), endPoint.y())); setStrokeStyle(savedStrokeStyle); setStrokeThickness(savedStrokeThickness); It works , and stroke thickness is effective. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) The fix for thickness seems to be as easy as saving the current thickness and setting the value to the thickness argument, and then restoring it, similarly to what is done for `StrokeStyle`. I did a quick test of that and the line thickness now matches Safari and Firefox. The positioning is off by what looks like 1/2 thickness, which would make sense if the values passed in were for the upper end of the (thick) line rather than the center, which is what drawLine expects. Adjusting both the start and end points by `thickness/2` makes WebView match the two browsers. So the following might be the fix for both the slanted lines, and the fact that we ignore thickness: StrokeStyle savedStrokeStyle = strokeStyle(); setStrokeStyle(stroke); float savedStrokeThickness = strokeThickness(); setStrokeThickness(thickness); FloatPoint startPoint = origin + FloatPoint(0, thickness / 2); FloatPoint endPoint = startPoint + FloatPoint(width, 0); drawLine( IntPoint(startPoint.x(), startPoint.y()), IntPoint(endPoint.x(), endPoint.y())); setStrokeStyle(savedStrokeStyle); setStrokeThickness(savedStrokeThickness); We would need to confirm my hypothesis that the position of the line should be adjusted like this. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) OK, I get what you are saying now. The dashed line case -- which is the only case we will go through that loop more than once -- has at least two problems: it doesn't adjust the origin point of the segments as it loops through the dash pattern (a functional bug that also is a performance hit), and we set and restore the stroke style inside the loop; probably the right fix is to not loop at all, but rather use the stroke setting for dashed lines and call drawLine once. I will ask Jay to file a follow-up issue for this so we can track it as a separate issue. For the problem with thickness, we either need to also file a new follow-up issue, or address it as part of this PR. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) thickness is probably specified in this CSS property * https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-thickness The fixed code also has a lot of problems, so We can understand how to fix it by comparing it with CoreGraphics. It will be up to the leader to decide when to make the remaining fixes. The source code history looks wrong when trying to correspond to thickness. * https://github.com/openjdk/jfx/blame/8955c2d5fe345d294d8a5ddba09fc23752a07c84/modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/GraphicsContextJava.cpp Another probably performance issue, ``` Java StrokeStyle savedStrokeStyle = strokeStyle (); setStrokeStyle (stroke); ``` Java setStrokeStyle (savedStrokeStyle); } This looks better outside the loop. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Sat, 12 Feb 2022 01:00:46 GMT, Kevin Rushforth wrote: > 2. The text is always drawn as a thin line... That should be "the text _underline or line-through_ is always drawn as a thin line...". - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) I don't follow the last point about using the same origin and overlapped lines being wasteful. It seems there are either two or three issues here. 1. Text underline and line-through are drawn slanted as described in this bug, which is a result of incorrectly adjusting the y coord one of the two end points passed into a `drawLine` call by the `thickness`. 2. The text is always drawn as a thin line, possibly because the stroke width doesn't take `thickness` into account (and while this could conceivably be done with `drawRect`, setting the stroke attributes and calling `drawLine` is the right way to do it). 3. MAYBE: there might be problem with drawing dashed line; if this doesn't work, the fix would probably be to set the dash pattern in the stroke. This PR addresses the first problem, which is the problem described in the bug report. The propose change looks correct. We could expand the bug report to cover at least the second case (since that is a somewhat related bug), but it might be better to file and fix that one separately (if it really is as simple as multiplying the existing stroke width by the thickness in the `drawLinesForText` method, then I wouldn't be opposed to fixing it in this PR, since it is somewhat related). In any case, I think the third issue of line pattern should be dealt with separately, if in fact, it is a problem. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) The discussion points are * Whether to use a simplified implementation that satisfies some of the specifications. * Whether to assume that lines will be broken. * * If you use the same origin, there is a waste that the line will be overlapped and the load will increase. Probably the following computational load * O(1/2*n*(n+1)) - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote:
> Issue: The end point of line in drawLinesForText , add thickness to the
> endPoint.y(). In this case origin which is start point and the end point
> would not be same, and line would be drawn not straight.
> Solution: Do not add thickness to the y position of end point of line.
> Start Point(x,y) --End Point(x + width, 0)
void GraphicsContextCG::drawLinesForText(const FloatPoint& point, float
thickness, const DashArray& widths, bool printing, bool doubleLines,
StrokeStyle strokeStyle)
{
if (!widths.size())
return;
Color localStrokeColor(strokeColor());
FloatRect bounds =
computeLineBoundsAndAntialiasingModeForText(FloatRect(point,
FloatSize(widths.last(), thickness)), printing, localStrokeColor);
if (bounds.isEmpty())
--
computeLineBoundsAndAntialiasingModeForText this will return rect after
considering thickness.
The TextDecorationPainter::paintTextDecoration..
FloatRect underlineBoundingBox =
m_context.computeUnderlineBoundsForText(rect, m_isPrinting);
DashArray intersections =
m_font.dashesForIntersectionsWithRect(textRun, textOrigin,
underlineBoundingBox);
DashArray boundaries =
translateIntersectionPointsToSkipInkBoundaries(intersections,
underlineBoundingBox.height(), rect.width());
ASSERT(!(boundaries.size() % 2));
// We don't use underlineBoundingBox here because
drawLinesForText() will run computeUnderlineBoundsForText() internally.
m_context.drawLinesForText(rect.location(), rect.height(),
boundaries, m_isPrinting, style == TextDecorationStyle::Double, strokeStyle);
This is already calculating underlineBoundingBox and generates boundaries.
m_context.drawLinesForText(rect.location(), rect.height(), boundaries,
m_isPrinting, style == TextDecorationStyle::Double, strokeStyle);
rect.height() is passed as thickness.
So, void GraphicsContextJava::drawLinesForText(const FloatPoint& origin, float
thickness, const DashArray& widths, bool, bool, StrokeStyle stroke) {
if (paintingDisabled())
return;
for (const auto& width : widths) {
// This is a workaround for http://bugs.webkit.org/show_bug.cgi?id=15659
StrokeStyle savedStrokeStyle = strokeStyle();
setStrokeStyle(stroke);
// do not add thickness to y position of end point , as line should be
straight to the origin
FloatPoint endPoint = origin + FloatPoint(width, 0);
drawLine(
IntPoint(origin.x(), origin.y()),
IntPoint(endPoint.x(), endPoint.y()));
setStrokeStyle(savedStrokeStyle);
}
}
To calculate end point , we should not add thickess to the end point. Let it
draw with default thickness.
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote:
> Issue: The end point of line in drawLinesForText , add thickness to the
> endPoint.y(). In this case origin which is start point and the end point
> would not be same, and line would be drawn not straight.
> Solution: Do not add thickness to the y position of end point of line.
> Start Point(x,y) --End Point(x + width, 0)
Looking at the GraphicsContexCG source, we can see that the
It looks correct to call fill rect instead of draw line.
The GraphicsContexJava implementation seems to be the cause of the underline
tilt.
Also, there seems to be no need to call drawLine() multiple times by referring
to last().
``` cpp
void GraphicsContextCG::drawLinesForText(const FloatPoint& point, float
thickness, const DashArray& widths, bool printing, bool doubleLines,
StrokeStyle strokeStyle)
{
if (!widths.size())
return;
Color localStrokeColor(strokeColor());
FloatRect bounds =
computeLineBoundsAndAntialiasingModeForText(FloatRect(point,
FloatSize(widths.last(), thickness)), printing, localStrokeColor);
if (bounds.isEmpty())
return;
bool fillColorIsNotEqualToStrokeColor = fillColor() != localStrokeColor;
Vector dashBounds;
ASSERT(!(widths.size() % 2));
dashBounds.reserveInitialCapacity(dashBounds.size() / 2);
float dashWidth = 0;
switch (strokeStyle) {
case DottedStroke:
dashWidth = bounds.height();
break;
case DashedStroke:
dashWidth = 2 * bounds.height();
break;
case SolidStroke:
default:
break;
}
for (size_t i = 0; i < widths.size(); i += 2) {
auto left = widths[i];
auto width = widths[i+1] - widths[i];
if (!dashWidth)
dashBounds.append(CGRectMake(bounds.x() + left, bounds.y(), width,
bounds.height()));
else {
auto startParticle = static_cast(std::ceil(left / (2 *
dashWidth)));
auto endParticle = static_cast((left + width) / (2 *
dashWidth));
for (auto j = startParticle; j < endParticle; ++j)
dashBounds.append(CGRectMake(bounds.x() + j * 2 * dashWidth,
bounds.y(), dashWidth, bounds.height()));
}
}
if (doubleLines) {
// The space between double underlines is equal to the height of the
underline
for (size_t i = 0; i < widths.size(); i += 2)
dashBounds.append(CGRectMake(bounds.x() + widths[i], bounds.y() + 2
* bounds.height(), widths[i+1] - widths[i], bounds.height()));
}
if (fillColorIsNotEqualToStrokeColor)
setCGFillColor(platformContext(), localStrokeColor);
CGContextFillRects(platformContext(), dashBounds.data(), dashBounds.size());
if (fillColorIsNotEqualToStrokeColor)
setCGFillColor(platformContext(), fillColor());
}
-
PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) [PageFillTest.java](https://github.com/openjdk/jfx/tree/master/tests/system/src/test/java/test/javafx/scene/web/PageFillTest.java) this example only for taking snapshot and read color property. It will not help to test line drawing. - PR: https://git.openjdk.java.net/jfx/pull/731
Re: RFR: 8280020: Underline and line-through not straight in WebView
On Thu, 10 Feb 2022 11:36:38 GMT, Jay Bhaskar wrote: > Issue: The end point of line in drawLinesForText , add thickness to the > endPoint.y(). In this case origin which is start point and the end point > would not be same, and line would be drawn not straight. > Solution: Do not add thickness to the y position of end point of line. > Start Point(x,y) --End Point(x + width, 0) The fix looks good. The test needs to be different, since it doesn't actually test this fix. A good test will fail before the fix and pass after the fix. Since you need to render something, it needs to be a system test, probably using snapshot. See [PageFillTest.java](https://github.com/openjdk/jfx/tree/master/tests/system/src/test/java/test/javafx/scene/web/PageFillTest.java) for a recent example. - PR: https://git.openjdk.java.net/jfx/pull/731
