On Thu, 27 Aug 2026 18:51:14 GMT, Andy Goryachev <[email protected]> wrote:
> Fixes broken navigation introduced in > [JDK-8370902](https://bugs.openjdk.org/browse/JDK-8370902) with non-zero > paragraph spacing (above, below, line spacing). > > Updated the Rich Editor Demo to allow setting paragraph line spacing in > addition to space above/below (right click -> Paragraph). > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Changes look good, tests pass with them. I've left some comments. apps/samples/RichTextAreaDemo/src/com/oracle/demo/richtext/editor/ParagraphDialog.java line 248: > 246: // FIX > 247: // weird issue: select all, right click, select Paragraph > 248: // change spacing, click OK to dismiss the dialog - In `FXUtils::setPopupMenu`, the contextMenu request adds an event filter to hide it when pressing on the owner node (the editor). When selecting Paragraph from the CM, dialog shows up, the CM hides, but the event filter remains. When the dialog closes, the first mouse click over the selection gets consumed by such filter. While the issue is not related to this PR (it only surfaces now that the style is applied to a selection), an easy fix could be added there, removing the event filter when the CM hides. Else, file an issue, and remove the comment? modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RangeInfo.java line 39: > 37: */ > 38: public final class RangeInfo { > 39: /// { miny, maxy, ... } remove extra slash? And it might be more meaningful like this? // { miny1, maxy1, miny2, maxy2,... } modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RangeInfo.java line 51: > 49: > 50: public static RangeInfo of(double width, double height) { > 51: return new RangeInfo(null, 0.0, height); Now that `d` is not defined, `width` is not used, maybe remove the parameter? modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RangeInfo.java line 60: > 58: List<TextLineInfo> lines = la.getTextLines(true); > 59: int sz = lines.size(); > 60: double[] d = new double[sz + sz]; it is the same, but better change `sz + sz` to `2 * sz`, to clearly indicate "two" elements per line (many, maxy)? modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RangeInfo.java line 69: > 67: // remove line spacing from the last line to force navigating to > the next cell > 68: if (sz > 0) { > 69: d[sz - 1] -= lineSpacing; if size of `d` is `2 * sz`, this should be d[2 * sz - 1] -=... ``` or `d[d.length - 1]`. modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RangeInfo.java line 96: > 94: > 95: public double findHitMidpoint(double y) { > 96: if (lines != null) { if y < lines[0], y is above the first line, i.e. the top padding space, or if y > lines[lines.length - 1], y is below the last line, the bottom padding space, then this method will return the midpoint of the entire paragraph, instead of a valid value, close to the y position (mid first line, or mid last line). modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/RichTextAreaBehavior.java line 684: > 682: double y = down ? > 683: ci.getMaxY() + ci.getLineSpacing() + deltaPixels + 1.0 : > 684: ci.getMinY() + deltaPixels - 1.0; Any reason for the change in those constants, worth a small comment? modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/TextCell.java line 424: > 422: } > 423: > 424: public boolean isOutsideTextRangeY(double y, boolean down) { `down` is not used modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/TextCell.java line 443: > 441: } > 442: > 443: public double findHitCandidate(double cellY, boolean down) { `down` is not used modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaNavigationTest.java line 189: > 187: > 188: /// tests vertical navigation in a randomly created text model with > all the combinations of > 189: /// controld width, start cursor position, text wrap, space > above/below, and line spacing. typo in `controld` (and extra slashes?) modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaNavigationTest.java line 226: > 224: > 225: int count = 0; > 226: for (;;) { add a limit in count, just in case? ------------- PR Review: https://git.openjdk.org/jfx/pull/2280#pullrequestreview-5072172266 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899143635 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899198078 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899222194 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899249794 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899265613 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899547973 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899343544 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899395843 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899398831 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899563466 PR Review Comment: https://git.openjdk.org/jfx/pull/2280#discussion_r3899575107
