RFR: 8280020: Underline and line-through not straight in WebView [v3]

[Kevin Rushforth]

On Sat, 19 Feb 2022 03:52:41 GMT, Jay Bhaskar <duke at openjdk.java.net> 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