On Tue, 7 Jul 2026 15:19:33 GMT, Andy Goryachev <[email protected]> wrote:
>> # Support for Embedded Images, Text Highlights, Wavy Underlines >> >> ## Summary >> >> This PR adds new attributes to support embedded images, text highlights, and >> wavy underlines. >> >> The `RichEditorDemoApp` gains new context menus and enables drag and drop to >> showcase the new functionality. >> >> <img width="768" height="703" alt="Screenshot 2026-06-18 at 12 40 28" >> src="https://github.com/user-attachments/assets/678bdbac-6582-46ee-bd41-ea938df930bb"; >> /> >> >> >> >> ## Problem >> >> Among the feedback received after publishing the RichTextArea incubating >> module, a number of users complained about the need to support embedded >> images. Other users complained about difficulty to get the text highlighted >> or wavy underlined, and having these decorations controlled via CSS. >> >> >> ## Solution >> >> Support for images was intentionally missing in the initial release; this PR >> adds the feature. >> >> Adding highlights and underlines is a bit more complicated, for several >> reasons. One reason is that the existing highlighting mechanism supports >> mixing of arbitrary number of translucent colors - something that is pretty >> much impossible to do with CSS alone. >> >> A useful compromise is to offer a limited number of standard attributes that >> can be controlled via CSS: 5 highlights and 3 wavy underlines. >> >> >> ## Specification >> >> >> ### StyleAttributeMap Attributes >> >> New attributes: >> - EMBEDDED_IMAGE >> - TEXT_HIGHLIGHT_1 ... TEXT_HIGHLIGHT_5 >> - UNDERLINE_WAVY_1 ... UNDERLINE_WAVY_3 >> >> ### CSS Styling >> >> The styling of highlights and wavy underlines is determined by the >> stylesheet and can be changed by the user. >> >> The style selectors are: >> >> `.rich-text-area .text-highlight-1` ... `.rich-text-area .text-highlight-5` >> `.rich-text-area .underline-wavy-1` ... `.rich-text-area .underline-wavy-3` >> >> >> ### New Public APIs >> >> `EmbeddedImage` >> `FileListFormatHandler` >> `StyleAttribute.inlineNode()` >> `StyleAttribute.isInlineNode()` >> `StyledSegment.ofInlineNode()` >> `CellContext.RunDecor` >> `CellContext.decorateRun()` >> `RichTextArea.dropTargetProperty` >> `RichTextArea.clearDropTarget()` >> `RichTextArea.getStyleAttributeMap()` >> >> >> ### File Format Changes >> >> The data transfer format (also used by the demo app to save the .rich files) >> version string is changed, making the old files unreadable. >> >> Perhaps it should remain unchanged for the duration of the incubation >> period. See also [2] >> >> >> ### Breaking API Changes >> >> `DataFormatHandler.createStyl... > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > demos I've reviewed the public API and it generally looks good. I left mostly javadoc comments and questions with a couple API questions. I haven't reviewed cssref yet, but that's next. doc-files/controls/RichTextArea/RichTextArea_DataFormat_Incubator.md line 41: > 39: > 40: ``` > 41: {@RichText-v4-incubator}{#tabs|99.5}{ff|System}{fs|12.0}tabs: 1 > 2 3{!tabs|149.0,190.0,229.0} Shouldn't that be "v3" -- not sure why it needs to jump from v2 to v4... doc-files/controls/RichTextArea/RichTextArea_DataFormat_Incubator.md line 129: > 127: |uw1 |UNDERLINE_WAVY_1 |boolean |Wavy underline color 1 > 128: |uw2 |UNDERLINE_WAVY_2 |boolean |Wavy underline color 2 > 129: |uw3 |UNDERLINE_WAVY_3 |boolean |Wavy underline color 3 I think `WAVY_UNDERLINE_N` (here and in other places) would be a more consistent naming. doc-files/controls/RichTextArea/RichTextArea_DataFormat_Incubator.md line 163: > 161: Example: > 162: > 163: `{img|w,138.0,h,102.0,tw,-1.0,th,-1.0,a,true,b,iVBORw0KGgoAAA...}` The example has `th,-1.0` but the above note says that -1 is only for the width. modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html line 6650: > 6648: underline-wavy-1<br> > 6649: underline-wavy-1<br> > 6650: underline-wavy-3 Copy / paste error: the second `-1` items should be `-2`. And here is another place where `wavy-underline-N` would be a more consistent name. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 526: > 524: > 525: /** > 526: * This property indicates the target text position during the > drag-and-drop operations. s/the drag-and-drop/drag-and-drop/ modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 550: > 548: * @since 27 > 549: */ > 550: public final void setDropTarget(double screenX, double screenY) { Should there be a variant of this method that takes a `TextPos`? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1493: > 1491: * @param pos the text position > 1492: * @param forInsert whether to pick preceding style at the segment > boundary > 1493: * @return the style attributes, non-null Minor: we usually say "never null" modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/DataFormatHandler.java line 45: > 43: * the implementation if the format contains no styles on its own > (for example, in the plain text format case). > 44: * > 45: * @param input the input data, never null Now that you take an Object instead of a String, you need to define what subtypes are expected and what happens if an unexpected type is passed in. Presumably "IllegalArgumentException" or "ClassCastException", but maybe it is a no-op? Some documentation should be on this abstract method. If the answer depends on the subclass, then document this with an `@implSpec` section. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/DataFormatHandler.java line 51: > 49: * @throws UnsupportedOperationException if the copy operation is not > supported > 50: */ > 51: public abstract StyledInput createStyledInput(Object input, > StyleAttributeMap attr) throws IOException; This needs `@since 27`, since it is a binary incompatible change that is effectively removing the old and adding a new method. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/EmbeddedImage.java line 91: > 89: > 90: /// Private constructor that DOES NOT make a defensive copy of the > bytes. > 91: private EmbeddedImage( Minor: We don't usually put javadoc comments on private methods of public classes. Suggestion: consider changing this to an ordinary inline comment (although I see that this pattern is used elsewhere, so this could be deferred). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/EmbeddedImage.java line 224: > 222: */ > 223: public EmbeddedImage copy(double targetWidth, double targetHeight, > boolean keepAspectRatio) { > 224: return new EmbeddedImage(bytes, width, height, targetWidth, > targetHeight, keepAspectRatio); Not an API issue, but this doesn't make a defensive copy. Is the `bytes` array ever modified? Or is it effectively deeply immutable? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/FileListFormatHandler.java line 68: > 66: @Override > 67: public StyledInput createStyledInput(Object input, StyleAttributeMap > attr) throws IOException { > 68: List<File> files = (List<File>)input; This is an example of what I mentioned in the superclass method. This class should provide docs and at least add an `@throws ClassCastException` (if that's what we decide is the cleanest behavior, else document the behavior you want and change the impl to check for instanceof and implement that behavior). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/FileListFormatHandler.java line 91: > 89: * @param files the list of files to be inserted > 90: */ > 91: public static void handleDrop(RichTextArea t, TextPos p, List<File> > files) { Who calls this static method? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextFormatHandler.java line 198: > 196: > 197: @Override > 198: public StyledInput createStyledInput(Object input, StyleAttributeMap > attr) { Check to see whether this inherits the `@since 27` modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextModel.java line 238: > 236: > 237: @Override > 238: public StyleAttributeMap getStyleAttributeMap(StyleResolver > resolver, TextPos pos, boolean forInsert) { Check whether `@since 27` inherits modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextModel.java line 427: > 425: * @since 27 > 426: */ > 427: public StyleAttributeMap getStyleAttributeMap(int charIndex, > boolean leading, boolean forInsert) { This is in a private class. It probably shouldn't have any javadoc, much less an `@since` modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 330: > 328: > 329: @Override > 330: public StyleAttributeMap getStyleAttributeMap(StyleResolver r, > TextPos pos, boolean forInsert) { Check `@since 27` modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyleAttribute.java line 87: > 85: */ > 86: public static <P> StyleAttribute<P> inlineNode(String name, Class<P> > type) { > 87: return new InlineNodeStyleAttribute<P>(name, type); Rather than a separate `inlineNode` factory method, could this instead be done with a check on the class type in `character()` factory method? I could ask the same of the other factory methods, so maybe there is some value in doing this way? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledSegment.java line 291: > 289: * Creates a StyledSegment which consists of a single inline Node. > 290: * @param generator the code to create a Node instance > 291: * @param a the segment styles, can be null What does "null" do? Is it different from an empty style attribute map? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledSegment.java line 294: > 292: * @return the StyledSegment instance > 293: */ > 294: public static StyledSegment ofInlineNode(Supplier<Node> generator, > StyleAttributeMap a) { Needs `@since 27` modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 254: > 252: * to the symbol indicated by the specified position. > 253: * <p> > 254: * This method returns the attributes of the last character at the > end of the document. This is a little confusing. My first reading of this sentence was that it always returned the attributes of the last character (that is, the character at the end of the document), which I'm sure isn't what you are trying to say. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 262: > 260: * @since 27 > 261: */ > 262: public abstract StyleAttributeMap getStyleAttributeMap(StyleResolver > resolver, TextPos pos, boolean forInsert); Rather than removing the existing method, would it be better to leave it and have it call the new method with a default value of forInsert (probably false, unless you think true is more common)? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 696: > 694: * This is a convenience method which eventually calls > 695: * {@link #replace(StyleResolver, TextPos, TextPos, StyledInput)} > 696: * with the attributes provided by {@link > #getStyleAttributeMap(StyleResolver, TextPos, boolean)} at the The doc should explicitly say that it calls `getStyleAttributeMap` with `true` for the last param. ------------- PR Review: https://git.openjdk.org/jfx/pull/2196#pullrequestreview-4648009869 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3538888293 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553884131 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553914744 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553927795 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554115897 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554127399 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554145633 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553852564 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553861047 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554009685 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554041286 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553949969 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553986613 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554413735 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554419752 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554433319 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554438949 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554378327 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553873911 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3553866580 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554178986 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554166523 PR Review Comment: https://git.openjdk.org/jfx/pull/2196#discussion_r3554187312
