8371067: RichTextArea: requestLayout by inline node doesn't reach VFlow by andy-goryachev-oracle · Pull Request #1975 · openjdk/jfx

andy-goryachev-oracle · 2025-11-17T21:46:06Z

Requesting VFlow re-layout when signaled by a Node embedded in TextCell.

NOTES

this PR depends on 8371069: RichTextArea: caret with inline node #1974 (resolved)
the fix can be verified visually using the latest standalone Monkey Tester https://github.com/andy-goryachev-oracle/MonkeyTest

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8371067: RichTextArea: requestLayout by inline node doesn't reach VFlow (Bug - P4)

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Ziad El Midaoui (@Ziad-Mid - Committer)

Reviewers without OpenJDK IDs

@Jugen (no known openjdk.org user name / role)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1975/head:pull/1975
$ git checkout pull/1975

Update a local copy of the PR:
$ git checkout pull/1975
$ git pull https://git.openjdk.org/jfx.git pull/1975/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1975

View PR using the GUI difftool:
$ git pr show -t 1975

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1975.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-17T21:46:32Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-17T21:47:18Z

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8371067: RichTextArea: requestLayout by inline node doesn't reach VFlow

Reviewed-by: arapte, zelmidaoui

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-11-18T16:25:51Z

Webrevs

Maran23 · 2025-11-19T08:54:27Z

+    // https://github.com/andy-goryachev/AppFramework/blob/1e9f2197ce510a77ec5f719a2cb7112b0b6cf7be/src/goryachev/fx/FX.java#L1081
+    // with the author's permission
+    /** returns a parent of the specified type, or null.  if node is an instance of the specified class, returns node */
+    public static <T> T getAncestorOfClass(Class<T> c, Node node) {


I would really recommend to not do anything like that. Normally, the layouting system, especially when requesting a layout should work and bubble up correctly. I wonder why this is needed. And if we found a special case, if we can solve it better.

Background: In the past projects, I often saw code like that and it turned out that this was never needed. It is often less readable and hurts the performance a bit.
We also improve coupling between components, which I would not recommend as well. Especially since subclasses can change a lot and it would be nice if everything still work out of the box.

Thank you @Maran23 for looking into this PR!

My experience is exactly opposite - I do use it often. The Node (or JComponent) hierarchy is a hierarchy, explicitly retaining the pointers to the each member's parent.

A specific member can be a child of a certain Parent, direct or otherwise, by design, and this method allows to get to that parent easily.

Let me ask you this - what is the alternative? Maintain a duplicate pointer?

Also, keep in mind this is not public API, it's a utility.

What I mean is:
Everything is part of the RichTextArea. Requesting a layout from a node inside should request a layout for each parent, also RichTextArea. So you should normally know that on the RichTextArea level, which also manages the VFlow. So it can do the corresponding actions, just by the node requesting the layout.

If we take a look at other more complex Controls, they do the following:

VirtualFlow.requestCellLayout -> sets a flag

layout is requested, and due to the flag, we know what to do

Maybe something that could be done here as well? Could also be done by Properties perhaps.

A specific member can be a child of a certain Parent, direct or otherwise, by design, and this method allows to get to that parent easily.
...
Also, keep in mind this is not public API, it's a utility.

Yes, we can always get the parent hierarchy. But that does not mean we should.
Making assumptions about the hierarchy will make subclasses and customizations (e.g. in the Skin) worse. If we extend RichTextArea and use another Node then VFlow, then we can expect TextCells not to work anymore?

In JavaFX, as you can also see in the codebase and other controls, retrieving an ancestor somewhere in the scene graph is pretty much never done or needed.
I did not have a look on this particular issue, but what I want to suggest is to take another look at the problem and how to solve it. So we don't need to rely on finding a specific node that might be somewhere in the scene graph.

If I remember correctly from what I've traced, is that requestLayout from the embedded node is propagating upwards until it reaches TextCell which extends a BorderPane. At this point requestLayout in Parent invokes markDirtyLayout with the forceParentLayout parameter/flag being false and the propagation stops.

I know why. Because the TextCell is not managed.

jfx/modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/TextCell.java

Line 70 in f87448e

setManaged(false);

Therefore, this will be set:

jfx/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java

Lines 1331 to 1334 in f87448e

boolean layoutRoot = false;

@Override final void notifyManagedChanged() {

layoutRoot = !isManaged() || sceneRoot;

}

Indeed, the TextCell will be handled as layout root, therefore not propagating any layout request further up.
So the real problem is, that the TextCell is not managed.

Otherwise the layout would be propagated to the RichTextArea, which can do the corresponding actions. So I would suggest looking into that.

The documentation on Node.managed property states

Defines whether or not this node's layout will be managed by its parent.

The layout of TextCell is not managed by its parent (VFlow.content), by design.

The VFlow is the entity that does the layout, for a reason. It's a complicated thing, and multiple moving parts are connected (one example: it performs multiple layout cycles in the same layout pass when scrollbars appear/disappear during the layout pass, to avoid jumping and flicker - something we occasionally see with ScrollPane and ListView, see Note 1).

Notes

I occasionally see the continuous flicker/layout when resizing the list of pages in the latest Monkey Tester, but so far I was unable to capture the exact conditions to create a reproducible test case (this is unrelated to this PR)

Did you read what I wrote above? I made multiple suggestions as how we can deal with the situation, even without making things managed again.

To reiterate, what is the problem?

We now have a dependency from TextCell to VFlow. Not explicitly, but rather hidden, by searching all parents (which is also costly when doing that often). I would prefer a clear separation of concerns

This makes subclassing/extending harder. What happens, if I want to write my own VFlow, but still use everything else, including TextCell? What if I want to use TextCell for a component, that has no VFlow? What happens if we find a VFlow from another component even? Because we used the TextCell somewhere else?

Other components made it clear how to use managed and unamanged nodes. And how to bubble up a request still. Why do we want to make an exception here? This is the first time we need to search the parent hierarchy

Why not e.g. binding to the needsLayoutProperty from a TextCell from within VFlow and just request the layout when set?

Also another point: The test is green for me with the changes from: #1945
So I would like to know, if maybe there was a bug even?

Thank you for looking into this and offering your suggestions! The thing is, the RTA is a bit more complex than the situation you describe, due to the presence of other requirements (such as side areas that house line numbers and possibly other paragraph decorators).

The design of the skin is such that only VFlow does the layout. The VFlow is the sole actor that deals with its complicated content. Without a very good reason, this design is unlikely to change. Furthermore, the VFlow is still an implementation detail, so one can't just subclass the skin (this is true for other controls as well). Neither VFlow nor TextCell nor RichUtils are public API.

Yes, the test is green with #1945 but it fails in master. And yes, the bug is still there even with #1945 , as can be seen with the monkey tester:

expected:

observed:

While looking at the test, discovered an issue with the SimpleViewOnlyStyledModel:
https://bugs.openjdk.org/browse/JDK-8372438

thanks!

I also had a similar concern as @Maran23.
But, after looking it more closely, It seems to be acceptable and needed solution for this scenario.

Few aspects:

Alternative approaches would be to save a reference to parent VFlow in each TextCell or a listener property. Which could be huge number, as there would be n TextCells (non-node embedding) under a VFlow.

The parent traversal is always fixed, i.e. only one time, as VFlow is immediate parent of a TextCell.

The re-layouts of VFlow are be limited, once a pulse. and at-least one is required.
3.1 even if multiple Node embedding TextCells get modified in a pulse.
3.2 even with multiple layout of a TextCell in a pulse

Overall this seems safe, with fixed negligible impact on performance, as traversal happens only for the modified TextCell. So, it would be one traversal for modified node(very likely one node in a frame/pulse). and alternative approaches would increase memory consumption depending on number of TextCells.

Jugen · 2025-11-20T13:08:37Z

     */
    public void add(Node node) {
        flow().getChildren().add(node);
+        embedsNode = true;


Is the intention that if the TextCell has no children (neither Text nor embedded nodes) that getAncestorOfClass is bypassed ?
Or is this to indicate that getAncestorOfClass should only be invoked if there are any embedded nodes (non Text ones) present ?
If the latter then note that add(Node) is called for both the adding of Text and embedded nodes and maybe you intended to overload the add method with add(Text) ?

All this is by design. VFlow is the component that lays out the text cells.

Umm, I find this code ambiguous - so just trying to clarify which of the two options presented you intend ?
If it's the first then maybe an alternative variable/flag name should be considered ?
If it's the second then embedsNode is always set true as soon as any content is added which means the first option is what is actually occurring ?

The expectation is that most text cells contain TextFlow, with the VFlow managing the layout. Once VFlow determines the target width, it queries the TextFlow for its preferred height to determine the actual size.

For paragraphs that contains Regions - either inline or as whole paragraphs - this logic needs to be augmented to propagate the requestLayout() flag up the hierarchy. We don't want to do this for pure text cells, but we must do it for embedded nodes.

addNode(Node) handles the inline node case, RichParagraph.of(Supplier<Region>) does the "full-width" case.

so to answer your question - if I understand the issue - the embedsNode flag was added to enable telling VFlow that it needs to reflow because some embedded node asked for it. Pure text cells don't need this signaling enabled.

Did I answer your question (satisfactorily :-) ?

I think so ....

Pure text cells don't need this signaling enabled.

If this is what is intended, then from how I read the code that is not what is happening because add(Node) is called even for Text only cells. See:

jfx/modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/VFlow.java

Lines 827 to 828 in f87448e

Text t = createTextNode(text, a);

cell.add(t);

you are right, thank you so much!

Thanks again @Jugen for pointing this out. Sorry, it took a while for me to understand what's going on.

Jugen · 2025-11-20T16:40:39Z

+            VFlow vf = RichUtils.getAncestorOfClass(VFlow.class, this);
+            if (vf != null) {
+                vf.requestLayout();
+            }


Would setNeedsLayout(true); maybe work here instead ?

no, we need to signal this to VFlow who does the layout.

andy-goryachev-oracle · 2025-11-20T19:07:45Z

/reviewers 2

openjdk · 2025-11-20T19:09:24Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

bridgekeeper · 2026-01-08T02:09:20Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2026-01-08T16:21:52Z

/touch

openjdk · 2026-01-08T16:23:57Z

@andy-goryachev-oracle The pull request is being re-evaluated and the inactivity timeout has been reset.

andy-goryachev-oracle · 2026-01-09T15:30:01Z

Could you please review this PR?
Reviewers: @kevinrushforth @Ziad-Mid

Jugen

lgtm

arapte

Overall looks good to me, with one query and a comment in the discussion that @Maran23 had started about traversal in getAncestorOfClass().

arapte · 2026-01-12T05:42:13Z

+        if (Window.class.isAssignableFrom(c)) {
+            Scene sc = node.getScene();
+            if (sc != null) {
+                Window w = sc.getWindow();
+                while (w != null) {
+                    if (w.getClass().isAssignableFrom(c)) {
+                        return (T)w;
+                    }
+
+                    // the window can be a dialog, check the owner
+                    if (w instanceof Stage stage) {
+                        w = stage.getOwner();
+                    }
+                }
+            }
+            return null;


It seems this if condition would never be true in current situation, and very likely be never true in future in context of RTA. A VFlow cannot be Window.
Can this code be removed ? keeping only the required else block.

You are right. This implementation was taken from another project I have that ensures that the right parent is returned, but in this particular case the Window- and Menu- specific code paths can be removed.

arapte · 2026-01-12T06:52:40Z

+    // https://github.com/andy-goryachev/AppFramework/blob/1e9f2197ce510a77ec5f719a2cb7112b0b6cf7be/src/goryachev/fx/FX.java#L1081
+    // with the author's permission
+    /** returns a parent of the specified type, or null.  if node is an instance of the specified class, returns node */
+    public static <T> T getAncestorOfClass(Class<T> c, Node node) {


I also had a similar concern as @Maran23.
But, after looking it more closely, It seems to be acceptable and needed solution for this scenario.

Few aspects:

Alternative approaches would be to save a reference to parent VFlow in each TextCell or a listener property. Which could be huge number, as there would be n TextCells (non-node embedding) under a VFlow.

The parent traversal is always fixed, i.e. only one time, as VFlow is immediate parent of a TextCell.

The re-layouts of VFlow are be limited, once a pulse. and at-least one is required.
3.1 even if multiple Node embedding TextCells get modified in a pulse.
3.2 even with multiple layout of a TextCell in a pulse

Overall this seems safe, with fixed negligible impact on performance, as traversal happens only for the modified TextCell. So, it would be one traversal for modified node(very likely one node in a frame/pulse). and alternative approaches would increase memory consumption depending on number of TextCells.

arapte

lgtm

andy-goryachev-oracle · 2026-01-13T16:30:59Z

@lukostyra could you please take a look at this, so it gets into jfx26?

Ziad-Mid

LGTM, tested in latest standalone MonkeyTester and new test added works fine.

andy-goryachev-oracle · 2026-01-13T22:19:38Z

/integrate

andy-goryachev-oracle · 2026-01-13T22:20:09Z

Thank you all for reviewing!

openjdk · 2026-01-13T22:21:08Z

Going to push as commit e20c0fd.

openjdk · 2026-01-13T22:21:16Z

@andy-goryachev-oracle Pushed as commit e20c0fd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

andy-goryachev-oracle added 4 commits November 13, 2025 16:01

fix

2fbc248

test

ee4c5b3

request layout

8758b2f

test

2459eb1

Merge branch 'master' into 8371067.request.layout

2725cf0

andy-goryachev-oracle marked this pull request as ready for review November 18, 2025 16:20

openjdk Bot added the rfr Ready for review label Nov 18, 2025

Maran23 reviewed Nov 19, 2025

View reviewed changes

Jugen reviewed Nov 20, 2025

View reviewed changes

andy-goryachev-oracle added 4 commits November 20, 2025 11:28

add text segment

b714faf

Merge branch 'master' into 8371067.request.layout

60de68a

updated test

ecd2c0e

Merge branch 'master' into 8371067.request.layout

56bcfff

andy-goryachev-oracle mentioned this pull request Dec 4, 2025

8373098: RichTextArea: NPE in InputMethodRequests #1997

Closed

3 tasks

Merge branch 'master' into 8371067.request.layout

ddd49b7

Merge branch 'master' into 8371067.request.layout

18c42d1

2026

c255d97

andy-goryachev-oracle requested review from Jugen and Maran23 January 9, 2026 15:28

kevinrushforth removed the request for review from Jugen January 9, 2026 15:58

kevinrushforth self-requested a review January 9, 2026 15:58

Jugen approved these changes Jan 9, 2026

View reviewed changes

arapte suggested changes Jan 12, 2026

View reviewed changes

andy-goryachev-oracle added 2 commits January 12, 2026 08:17

Merge branch 'master' into 8371067.request.layout

08e3fde

review comments

d605d6b

arapte approved these changes Jan 13, 2026

View reviewed changes

andy-goryachev-oracle requested a review from Jugen January 13, 2026 15:17

Jugen approved these changes Jan 13, 2026

View reviewed changes

Ziad-Mid approved these changes Jan 13, 2026

View reviewed changes

openjdk Bot added the ready Ready to be integrated label Jan 13, 2026

openjdk Bot added the integrated Pull request has been integrated label Jan 13, 2026

openjdk Bot closed this Jan 13, 2026

openjdk Bot removed ready Ready to be integrated rfr Ready for review labels Jan 13, 2026

andy-goryachev-oracle deleted the 8371067.request.layout branch January 13, 2026 22:23

	boolean layoutRoot = false;
	@Override final void notifyManagedChanged() {
	layoutRoot = !isManaged() \|\| sceneRoot;
	}

Conversation

andy-goryachev-oracle commented Nov 17, 2025 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTES

Progress

Issue

Reviewers

Reviewers without OpenJDK IDs

Reviewing

Uh oh!

bridgekeeper Bot commented Nov 17, 2025

Uh oh!

openjdk Bot commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maran23 Nov 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

arapte Jan 12, 2026 • 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!

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 Nov 20, 2025

Uh oh!

openjdk Bot commented Nov 20, 2025

Uh oh!

bridgekeeper Bot commented Jan 8, 2026

Uh oh!

andy-goryachev-oracle commented Jan 8, 2026

Uh oh!

openjdk Bot commented Jan 8, 2026

Uh oh!

andy-goryachev-oracle commented Jan 9, 2026

Uh oh!

Jugen left a comment

Choose a reason for hiding this comment

Uh oh!

arapte left a comment

Choose a reason for hiding this comment

Uh oh!

arapte Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

andy-goryachev-oracle commented Nov 17, 2025 •

edited by openjdk Bot

Loading

openjdk Bot commented Nov 17, 2025 •

edited

Loading

mlbridge Bot commented Nov 18, 2025 •

edited

Loading

Maran23 Nov 20, 2025 •

edited

Loading

arapte Jan 12, 2026 •

edited

Loading

arapte Jan 12, 2026 •

edited

Loading

arapte Jan 12, 2026 •

edited

Loading