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

[Kevin Rushforth]

On Fri, 24 Oct 2025 05:33:45 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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...

@hjohn Thank you for providing the additional analysis. This will help those of us who will review this proposed fix evaluate it to see if we can poke any holes in it.

As you say, we have two choices before us: fix this bug or revert the fix for [JDK-8360940](https://bugs.openjdk.org/browse/JDK-8360940). If we can convince ourselves that this is the right fix, taking this fix would be preferable to re-introducing JDK-8360940.

As for testing, I think manual testing using various apps (e.g., Ensemble, MonkeyTester, SceneBuilder) is needed. And while we also need a new test for _this_ fix -- that is, a test that fails before and passes after the fix -- I would agree that adding a whole battery of functional tests for layout is out of scope and better done as a follow-on.

Finally, as you point out, we could provide a system property to go back to the way we did it before. If we do that (which I am not advocating at this point in the review), we might consider having that flag also revert the behavior of [JDK-8360940](https://bugs.openjdk.org/browse/JDK-8360940).

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

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