8341670: [Text,TextFlow] Public API for Text Layout Info #1596

andy-goryachev-oracle · 2024-10-08T16:07:54Z

Please refer to

https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Text/LayoutInfo.md

The RichTextArea control (JDK-8301121), or any custom control that needs non-trivial navigation within complex or wrapped text needs a public API to get information about text layout.

This change fixes the missing functionality by adding a new public method to the Text and TextFlow classes.:

    /**
     * Obtains the snapshot of the current text layout information.
     * @return the layout information
     * @since 25
     */
    public final LayoutInfo getLayoutInfo()

The LayoutInfo provides a view into the text layout within Text/TextFlow nodes such as:

caret information
text lines: offsets and bounds
overall layout bounds
text selection geometry
strike-through geometry
underline geometry

WARNINGS

Presently, information obtained via certain Text/TextField methods is incorrect with non-zero padding and borders, see JDK-8341438.

This PR provides correct answers in the new API, leaving the behavior of the existing methods unchanged (there is a compatibility risk associated with trying to fix JDK-8341438 ).

Also, the RTL support is out of scope for this PR, due to multiple pre-existing conditions, see https://bugs.openjdk.org/browse/JDK-8343557

Testing

The new APIs can be visually tested using the Monkey Tester on this branch:
https://github.com/andy-goryachev-oracle/MonkeyTest/tree/text.layout.api

in the Text and TextFlow pages:

Two very basic headful tests have been added.

Why an interface ? It is easier to evolve a class when you want to add more info.
Also it should be sealed unless you can show why it is important that
applications can create instances.

It is being exposed for the benefit of RT but the class doc itself doesn't say when you might want to get one and why.
It might also benefit from a succinct description of what "the text layout" means and encompasses.

Oh and how do you know when it is no longer valid ?

good points, thanks! please let me know if the update addressed your concerns.

a couple of notes:

converted to an abstract class. I feel weird referring to an internal implementation at the API level (public sealed abstract class LayoutInfo permits com.sun.javafx.text.PrismLayoutInfo, yikes!)

why we get it: we don't explain why we need HitInfo or caret shapes or range shapes, do we?

prrace · 2024-10-09T19:30:39Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+/**
+ * Represents an immutable snapshot of certain aspects of the text layout
+ * in a {@code Text} or {@code TextFlow} node,
+ * such as break up of the text into lines and the their bounds.


"the their"

prrace · 2024-10-09T19:44:30Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+ * such as break up of the text into lines and the their bounds.
+ * <p>
+ * The snapshot is valid until the layout changes due to any change that
+ * triggers that, such as resizing of the container or modification of properties.


"until the layout changes due to any change that triggers that"

it's not wrong, but writing change twice reads a bit off.
How about

The layout snapshot is no longer valid after actions such as resizing of the container, or modification of certain properties.
For example updating the text or the font would invalidate the layout snapshot, but a change of color would not.

I still don't see how applications can KNOW this has happened.
There's nothing on the (immutable) layout which says its invalidated.
There's no property to monitor.
So people will in effect never be able to do anything but use it and toss it and get a new one every time.
Perhaps you can make it 'cheap' to do this.
Since it is immutable, the Text or TextFlow can cache it.
If it is invalidated, the Text or TextFlow should know and can flush its cached version.
So the SAME immutable instance can be returned over and over again until that happens.

People can then also use "==" or equals() to check this and save themselves validation too.
I notice you don't have equals() so probably that is needed anyway to save people from manual comparison .. also an evolution issue if the class is updated.

A consequence of that is then the text about invalidation can be updated with advice on doing a simple equals() call if they need to know if it changed. And equals will start with "==" so will be really fast to return true ..

thank you!

I am struggling with how to explain that it should neither be cached, nor compared to another snapshot.

Maybe an example would help. Use case is an editable rich text component based on TextFlow. The user presses HOME and the caret needs to go to the beginning of the line. We get the LayoutInfo, determine which line the caret is currently at and determine the start offset of that line to move the caret to.

Another example: in Text, to determine whether the mouse is clicked inside of the text or beyond the last character (see JDK-8091012 ).

We are not expecting the application to monitor the text layout, so no notifications or observable properties or equals() are needed.

(I've added some words to that effect in Text/TextFlow.getLayoutInfo() )

Clients will usually obtain a LayoutInfo in order to respond to some user action or an update of the target node.
LayoutInfo should be considered an ephemeral snapshot, to be used and immediately discarded.
Clients should always obtain a new one when they need to respond again as likely the trigger to respond again means the target node has changed.

Although I don't see the harm in being more efficient.

I hear a suggestion to create a think wrapper/proxy instead of the snapshot, right?

This might, in fact, be a better idea. The API stays the same.
Let me think.

Thanks!

andy-goryachev-oracle · 2024-10-10T17:16:57Z

@Jugen do you think this PR addresses the issue https://bugs.openjdk.org/browse/JDK-8091012 ?

andy-goryachev-oracle · 2024-10-10T19:23:47Z

One additional thought/question:

There is a need to extract a few more data points from the text layout in addition to the text lines, specifically:

bounds of the (selection) range, as rectangles
strikethrough range shape (as PathElement[] and/or rectangles)
underline range shape (as PathElement[] and/or rectangles)

We currently have Text/TextFlow.underlineShape(int,int), .rangeShape(int,int) which return PathElement[] in an undocumented way.

There is no corresponding .strikeThroughShape() method, or a way to retrieve the geometric bounds as lines/rectangles.

Granted, one can attempt to analyze the PathElement[] and convert it to geometry, but it seems we should avoid asking the clients to do that.

So the question are:

should we add these data points to LayoutInfo
if so, in what form? i.e. as individual methods or as an array of Line/Rectangle objects,
should we add the .strikeThroughShape() to Text/TextFlow, or simply adding it here should be sufficient?

Jugen · 2024-10-11T13:57:02Z

@andy-goryachev-oracle First off thanks for all your work on the richtext control you're working on.

@Jugen do you think this PR addresses the issue https://bugs.openjdk.org/browse/JDK-8091012 ?

Unfortunately I don't think this helps for the hit problem Tomas was having, see hit(x,y) in RichTextFX's TextFlowExt for the relevant code.

However I was able to use all the methods in the LayoutInfo interface of this PR in RichTextFX to replace the custom layout info code, which is great.

Jugen · 2024-10-11T14:22:27Z

Wrt to rangeShape & underlineShape: RichTextFX quite successfully uses the PathElement[] returned to it's advantage and so wouldn't gain from Rectangles.

.strikeThroughShape() should be added to Text/TextFlow for consistency sake.

andy-goryachev-oracle · 2024-10-11T17:03:58Z

thank you so much @Jugen for checking and the feedback!

I see that a lot of what is being done in TextFlowExt should become unnecessary with the new API (or maybe even the whole thing might become unnecessary).

Also, while the new API do not address JDK-8091012 directly, that is, there is no field added to HitInfo, it looks like the information provided by the LayoutInfo should be sufficient to implement what you guys need as a utility, since you will have the geometry of text lines in the layout, right?

Anyway, thank you. I agree with you that we should add T/TF.strikeThroughShape() for consistency sake.

andy-goryachev-oracle · 2024-10-11T17:15:43Z

Thank you you all @prrace @Jugen @kevinrushforth for the feedback!
I am going to take this PR back to draft to make the suggested changes.

andy-goryachev-oracle · 2024-10-11T22:27:46Z

/issue add JDK-8341672

openjdk · 2024-10-11T22:28:11Z

@andy-goryachev-oracle
Adding additional issue to issue list: 8341672: [Text/TextFlow] getRangeInfo.

andy-goryachev-oracle · 2025-05-02T17:22:42Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java

+     * @return an array of {@code PathElement} which can be used to create a {@code Shape}
+     * @since 25
+     */
+    public final PathElement[] strikeThroughShape(int start, int end) {


Perhaps this method should be renamed to getStrikeThroughShape() to be consistent with the proposed evolution of the buggy methods in https://bugs.openjdk.org/browse/JDK-8341438

(same in Text)

andy-goryachev-oracle · 2025-05-02T17:46:26Z

I'll update the CSR once the javadoc settles.

kevinrushforth

The changes to the API look good. I left a few wording suggestions. Once those are resolved, update the CSR.

I see that this PR lists JDK-8341671 and JDK-8341672 as additional issues solved. They are included as part of the text LayoutInfo, so it would be better to close them as duplicates and remove them from this PR (using /issue remove).

kevinrushforth · 2025-05-21T20:51:07Z

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java

+     * <p>
+     * While there is no general guarantee that successive invocations of this method return the same instance,
+     * it is safe to either cache this object or call this method each time, since the information obtained from
+     * this lightweight object remains valid until the next layout cycle.


Oh, so it isn't a view but a copy. I think what you changed it to is closer. The "object which provides a snapshot" is a little misleading, though. It is this method that provides the snapshot. The returned TextLayout object is the snapshot. Maybe something like this?

* Returns a copy of the of the text layout geometry for this node. This copy is a snapshot * of the text layout at the time the method is called.

kevinrushforth · 2025-05-21T20:53:22Z

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java

+     * it is safe to either cache this object or call this method each time, since the information obtained from
+     * this lightweight object remains valid until the next layout cycle.
+     *
+     * @return the layout information


maybe "a copy of the ..."?

kevinrushforth · 2025-05-21T20:53:52Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java

@@ -698,4 +714,28 @@ public TextLayout getTextLayout(TextFlow f) {
            }
        });
    }
+
+    /**
+     * Returns the object which provides a snapshot of the text layout geometry for this node.


Same comment as for Text::getLayoutInfo

kevinrushforth · 2025-05-21T20:58:08Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+import com.sun.javafx.text.PrismLayoutInfo;
+
+/**
+ * Provides a snapshot of the text layout geometry in a {@code Text} or a {@code TextFlow} node,


I might either remove the word "Provides" or else replace it with "Holds".

andy-goryachev-oracle · 2025-05-21T22:53:11Z

/issue remove JDK-8341671

openjdk · 2025-05-21T22:53:21Z

@andy-goryachev-oracle
Removing additional issue from issue list: 8341671.

andy-goryachev-oracle · 2025-05-21T22:53:29Z

/issue remove JDK-8341672

openjdk · 2025-05-21T22:54:12Z

@andy-goryachev-oracle
Removing additional issue from issue list: 8341672.

kevinrushforth

Updated API looks good. I left a comment on the CSR.

I left a comment on one other naming / semantic issue that I just noticed.

kevinrushforth · 2025-05-22T00:05:29Z

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java

+     * @return an array of {@code PathElement} which can be used to create a {@code Shape}
+     * @since 25
+     */
+    public final PathElement[] getStrikeThroughShape(int start, int end) {


I just noticed that this is named getStrikeThroughShape rather than strikeThroughShape. The latter would match the similar rangeShape, caretShape, and underlineShape methods. We had discussed that you might propose to fix JDK-8341438 by deprecating rangeShape and underlineShape and adding new getRangeShape and getUnderlineShape methods (although that might be problematic given the caretShape property, which necessarily already has a no-arg getCaretShape() method).

Did you name this method getStrikeThroughShape with the intention that it would have the fixed semantics of applying the insets from its inception? If so, then I recommend documenting that fact in this method.

Yes, you are right - and I plan to address this in https://bugs.openjdk.org/browse/JDK-8341438 (as it will be a separate change).

We discussed this with @kevinrushforth - I am going to remove the getStrikeThroughShape() method from Text/TextFlow in this PR, and add it under either a separate ticket, or as a part of https://bugs.openjdk.org/browse/JDK-8341438

Created https://bugs.openjdk.org/browse/JDK-8357594 for Text::strikeThroughShape and TextFlow::getStrikeThroughShape.

mstr2 · 2025-05-22T21:47:22Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismLayoutInfo.java

+    public List<TextLineInfo> getTextLines(boolean includeLineSpacing) {
+        TextLine[] lines = layout.getLines();
+        Insets m = insets();
+        double dx = m.getLeft(); // TODO rtl?


This comment looks like it's a question to the reader. Does the code not work for RTL layouts?

JavaFX does not support RTL fully - there is a whole series of issues waiting to be fixed.
See https://bugs.openjdk.org/browse/JDK-8343557

kevinrushforth

The API looks good given the removal of the Text/TextFlow getStrikeTrhoughShape methods. I added one question about whether you want to consider adding a getter to CaretInfo to return the list of rectangles in a single call, but I'll leave that up to you.

I pointed out what I think are a couple doc typos in TextLine info.

I think this is close to being ready. I'll review the implementation and do some testing.

kevinrushforth · 2025-06-05T21:00:49Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextLineInfo.java

+ *     for left-aligned text.
+ *   <li>
+ *     {@code minY} - the ascent of the line (negative).
+ *     The ascent of the line is the max ascent of all fonts in the line.


Shouldn't that be "glyphs" not "fonts"?

This definition is copied from com.sun.javafx.scene.text.TextLine::getBounds L58.
I'll let Phil confirm that, but since it's a max ascent, it might be a function of the font rather than the particular glyphs.

kevinrushforth · 2025-06-05T21:01:52Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextLineInfo.java

+ *   <li>
+ *     {@code height} - the height of the line.
+ *     The height of the line is sum of the max ascent, max descent, and
+ *     max line gap of all the fonts in the line.


Shouldn't that be "glyphs" not "fonts"?

kevinrushforth · 2025-06-05T21:02:29Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextLineInfo.java

+ *     The ascent of the line is the max ascent of all fonts in the line.
+ *   <li>
+ *     {@code width} - the width of the line.
+ *     The width for the line is sum of all the run widths in the line, it is not


minor: "for the line" --> "of the line"

kevinrushforth · 2025-06-05T21:04:43Z

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java

+     *
+     * @return the number of segments representing the caret
+     */
+    public abstract int getSegmentCount();


Do you think it's worth adding a List<Rectangle2D>getSegments() method in addition to (or instead of) getSegmentCount and getSegmentAt?

Probably unnecessary, because the caret has one segment most of the time. When bidi is involved, the caret might have 2 segments. It will never have 3 or more segments.

kevinrushforth · 2025-06-05T21:15:52Z

@prrace Can you also review the API changes?

draft api

285db28

openjdk bot added the csr Need approved CSR to integrate pull request label Oct 8, 2024

bounds

43ac0e1

andy-goryachev-oracle marked this pull request as ready for review October 8, 2024 18:16

openjdk bot added the rfr Ready for review label Oct 8, 2024

prrace reviewed Oct 9, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits October 9, 2024 12:13

review comments

21dbe6c

javadoc

04c4abb

prrace reviewed Oct 9, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits October 9, 2024 13:41

clarify

6869c76

convert to wrapper

2009a7e

andy-goryachev-oracle marked this pull request as draft October 11, 2024 17:15

openjdk bot removed the rfr Ready for review label Oct 11, 2024

andy-goryachev-oracle added 2 commits October 11, 2024 10:33

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

6b7b079

text line info

c2e26d5

andy-goryachev-oracle marked this pull request as ready for review October 11, 2024 22:29

openjdk bot added the rfr Ready for review label Oct 11, 2024

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

6782658

andy-goryachev-oracle commented May 2, 2025

View reviewed changes

review comments

982ef1f

andy-goryachev-oracle requested review from mstr2, kevinrushforth and jperedadnr May 2, 2025 17:45

kevinrushforth reviewed May 21, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits May 21, 2025 15:44

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

f5781cd

review comments

0ac75da

andy-goryachev-oracle requested a review from kevinrushforth May 21, 2025 23:01

kevinrushforth reviewed May 22, 2025

View reviewed changes

andy-goryachev-oracle added 4 commits May 22, 2025 10:15

caret geometry

c885173

removed getStrikeThroughShape

0f02fe9

cleanup

34b0646

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

ab8bd9c

andy-goryachev-oracle requested a review from kevinrushforth May 22, 2025 19:14

mstr2 reviewed May 22, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits May 27, 2025 07:50

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

f4fb845

cleanup

de1016b

andy-goryachev-oracle mentioned this pull request May 29, 2025

8341438: TextFlow: incorrect caretShape(), hitTest(), rangeShape() with non-empty padding/border #1817

Draft

4 tasks

kevinrushforth reviewed Jun 5, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits June 5, 2025 14:32

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

972c0d7

review comments

d0f56fe

andy-goryachev-oracle requested a review from kevinrushforth June 5, 2025 21:39

+               *
+               * @since 24
+               */
+              public interface LayoutInfo {

8341670: [Text,TextFlow] Public API for Text Layout Info #1596

Are you sure you want to change the base?

8341670: [Text,TextFlow] Public API for Text Layout Info #1596

Uh oh!

Conversation

andy-goryachev-oracle commented Oct 8, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WARNINGS

Testing

See Also

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 8, 2024

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

mlbridge bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

prrace Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prrace Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Oct 10, 2024

Uh oh!

andy-goryachev-oracle commented Oct 10, 2024

Uh oh!

Jugen commented Oct 11, 2024

Uh oh!

Jugen commented Oct 11, 2024

Uh oh!

andy-goryachev-oracle commented Oct 11, 2024

Uh oh!

andy-goryachev-oracle commented Oct 11, 2024

Uh oh!

andy-goryachev-oracle commented Oct 11, 2024

Uh oh!

openjdk bot commented Oct 11, 2024

Uh oh!

andy-goryachev-oracle May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented May 2, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Oct 8, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 8, 2024 •

edited

Loading

prrace Oct 9, 2024 •

edited

Loading

prrace Oct 9, 2024 •

edited

Loading

andy-goryachev-oracle May 2, 2025 •

edited

Loading

kevinrushforth left a comment •

edited

Loading