RFR: 8370498: Improve how Node detects whether a layout property change requires a new layout pass [v2]

[John Hendrikx]

On Thu, 23 Oct 2025 16:55:32 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> This new check is much more accurate to detect whether a parent is currently laying out its children. The previous code almost never worked, resulting in additional unnecessary layouts.
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix ToolBarSkinTest
>   
>   Reusing a toolbar as part of several scenes, in combination with the StubToolkit that doesn't handle pulses makes this test fail with the relayout detection fix.

> _Mailing list message from [Andy Goryachev](mailto:andy.goryachev at oracle.com) on [openjfx-dev](mailto:openjfx-dev at mail.openjdk.org):_
> 
> > We will need a unit test for this.
> 
> Perhaps we could even get it a bit further than that. What would you say could be a sufficiently comprehensive set of tests/scenarios to reduce the probability of regression?
> 
> Let's say, we limit the depth of the hierarchy to 3 (node-parent-parent), then the combinatorial complexity should still be manageable.
> 
> What do you think?

As much as I'd like to help out more, after analyzing this problem already for almost two full days, I don't have time to write a comprehensive test suite for one of the more complex systems in JavaFX.  I can of course extend existing tests to get behavioral coverage on this new fix.

So, I think we then should choose what we want.  We can revert https://bugs.openjdk.org/browse/JDK-8360940 and go back to a situation where the layout system can easily get into a bad state while using documented API's, or we can take a critical look at this new fix, which is fixing a long standing problem that was hidden by the bug that was fixed, and could be a major performance improvement for complex UI's.

I think manual testing with some large UI's and/or Monkey Tester should be more than sufficient to discover if this is likely to cause regressions, and I've already been doing so before I submitted this fix (I tend to just apply these fixes to my own code directly, to see how they work out).  

If we're still unsure, a system property can turn this on/off easily, at the cost of getting some redundant layout passes in the "off" mode (as it was before).

I already knew that the detection that `Node` was doing for triggering a 2nd layout pass was bogus, because `isCurrentLayoutChild` is simply not updated at the correct moments for this to work.  It works if you ask if a Node's **parent** is the current layout child of the **grand parent**, but it does not work for one level deeper: asking if a node is the current layout child of its parent.

That the logic looked correct at first glance may have have convinced the original authors that it does work, but it doesn't.  More importantly, it can't be fixed as a fix would require **each** modification of layout X/Y to wrap this change by setting the current layout child first -- that would require updating all layout containers, all 3rd party layout containers, and all 3rd party code that calls `relocate` or sets layout X/Y directly -- an impossibility IMHO.  That's also the reason I didn't fix that immediately, as at the time I didn't see a workable solution.

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1945#issuecomment-3441126783