[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8280020: Underline and line-through not straight in WebView [v5]
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2022-02-28 15:20:54
Message-ID: pqEuz9sTOxbn7R7q_rdTZxnvjZ4H-89f5j78HVBjUNg=.48d079f4-a783-4308-a99d-d0c2e0c69724 () github ! com
[Download RAW message or body]

On Sun, 27 Feb 2022 07:10:25 GMT, Jay Bhaskar <duke@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: 
> 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


[prev in list] [next in list] [prev in thread] [next in thread]