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.

So I've been playing with the program in https://bugs.openjdk.org/browse/JDK-8137252 and here are some observations:

- It is binding width/height of a sibling to another (size altering) property of another sibling.  This is questionable practice, especially when both siblings are **managed** and part of a container that uses managed children (StackPane in this case).  You get away with this because `Shape`s are not resizable (by the standard width/height mechanism), and so the container can't force its a preferred size onto a `Shape`.  If it was anything else (like a Region or Button) then after changing the size, relayout would run, and your change would be overridden, at least, until you set that control to unmanaged.

- Because containers do not resize non-resizable controls, but do set their positions, this is a very special half managed/half unmanaged territory.

- The whole thing works by doing a **2nd** layout pass.  This means that when you modify the width/height of the source sibling, that this will trigger a first layout pass.  During the layout pass, you modify the values of the destination sibling.  This then requires another layout pass.  Two layout passes aside, this will show up on your screen as the **layout "jumping"** (ie. it shows the destination sibling first in the wrong position, then in the 2nd pass it corrects this).  I don't see why anyone would want to do it this way, as this will make the layout jump.  In fact, if you go further, and bind more properties in these ways, you can get a 3rd layout pass, and a 4th etc.  This is not a good solution.

Currently, on each change (showing the stage, changing the source sibling's size) we see the layoutX of the destination sibling jumping:

          lx = 0.0
          Showing scene
          lx = 9.333332697550457
          lx = 17.99999936421712
          Triggering 2nd change
          lx = 17.333334604899086
          lx = 34.00000127156576
          
> What's the correct way then to draw a circle around a label that doesn't make your layout jump?

The wanted solution does not make it easy.  Because the label's width is used as radius, the desired circle actually is twice as wide as the label.  The stack pane however is unaware of this requirement as the circle is not resizable and does not have min/pref/max properties.  It has some similarities with solutions that must know the size of text (in the correct font after CSS has been applied) because other elements depend on it.

The solution is to add a listener to the label's `needsLayoutProperty` (which BTW is undocumented but public since 2012).  Every time something of consequence changes on the Label, one can now calculate how large the circle should be.  It is a bit more involved, but you get a **single** layout pass, and there is no UI jumping anymore:


public class Test1 extends Application {

    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) {

        Label l1 = new Label("1 2");
        Ellipse e1 = new Ellipse(100, 50);
        StackPane sp = new StackPane(l1, new EllipseWrapper(l1));

        e1.setOpacity(0.5);
        sp.relocate(200, 100);

        Pane topPane = new Pane(sp);
        Scene scene = new Scene(topPane, 600, 400);

        sp.setStyle("-fx-border-color: RED;");

        stage.setScene(scene);
        System.out.println("Showing scene");

        stage.show();

        Thread.ofVirtual().start(() -> {
          for(;;) {
            try {
              Thread.sleep(1500);
            }
            catch(InterruptedException e) {
              break;
            }

            Platform.runLater(() -> {
              l1.setText("" + (int)(2000 * Math.random()));
            });
          }
        });
    }

    // wrapper because Ellipse has no layout properties for StackPane to use:
    class EllipseWrapper extends Region {
      private final Ellipse ellipse = new Ellipse();
      private final Label label;

      public EllipseWrapper(Label label) {
          this.label = label;
          ellipse.setOpacity(0.5);
          getChildren().add(ellipse);

          // using a change listener to ensure property is revalidated:
          label.needsLayoutProperty().subscribe(v -> requestLayout());
      }

      @Override
      protected double computePrefWidth(double height) {
          return 2 * label.prefWidth(-1);  // proper calculation for layout!
      }

      @Override
      protected double computePrefHeight(double width) {
          return 2 * label.prefHeight(-1);
      }

      @Override
      protected void layoutChildren() {
          double w = getWidth();
          double h = getHeight();
          ellipse.setRadiusX(w / 2);
          ellipse.setRadiusY(h / 2);
          ellipse.setCenterX(w / 2);
          ellipse.setCenterY(h / 2);
      }
  }
}

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

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