JDK-8137252 : JavaFX StackPane bounds not updated
  • Type: Bug
  • Component: javafx
  • Sub-Component: scenegraph
  • Affected Version: 8u51,9
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: windows_7
  • CPU: x86_64
  • Submitted: 2015-09-27
  • Updated: 2017-02-02
  • Resolved: 2016-03-11
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.8.0_51"
Java(TM) SE Runtime Environment (build 1.8.0_51-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.51-b03, mixed mode)


ADDITIONAL OS VERSION INFORMATION :
Windows 64bit ver 7

A DESCRIPTION OF THE PROBLEM :
When I run the following program

import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.layout.Pane;
import javafx.scene.layout.StackPane;
import javafx.scene.shape.Ellipse;
import javafx.stage.Stage;


public class Test1 extends Application {

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

    @Override
    public void start(Stage stage) throws Exception {       
        Pane topPane = new Pane();      
        Scene scene = new Scene(topPane, 600, 400);

        StackPane sp = new StackPane();
        Label l1 = new Label("1 2");
        Ellipse e1 = new Ellipse(100, 50);
        e1.setOpacity(0.5);
        sp.getChildren().addAll(l1, e1);            
        e1.radiusXProperty().bind(l1.widthProperty());
        e1.radiusYProperty().bind(l1.heightProperty());         
        topPane.getChildren().add(sp);
        sp.relocate(200, 100);

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

        Platform.runLater(() -> {
            //l1.setText("123");
            //l1.setText("1 2");
        });             

        stage.setScene(scene);
        stage.show();       
    }
}

I get a red box surrounding the text label only, but when I uncomment the two lines inside the Platform.runLater() block above, I get a red box surrounding the outer ellipse, which is what I want. So it seems to me the layout bounds of the stack pane is not set correctly from the model description, since the bounds are determined only from the label control. But when I force an invalidation in Platform.runLater() the layout bounds are where they should be.
Why is this happening and how do I prevent it? I would like to be able to just specify my model/graph and then it should render correctly on the first show, or?


Also see stackexchange question http://stackoverflow.com/questions/32271672/javafx-stackpane-bounds-not-updated



ADDITIONAL REGRESSION INFORMATION: 
java version "1.8.0_51"
Java(TM) SE Runtime Environment (build 1.8.0_51-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.51-b03, mixed mode)


STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Run the program in the description and it will happen.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
I expect to see a a red box border surrounding the outer ellipse, as specified in the model. 
ACTUAL -
I see a red box surrounding the label. But if I uncomment the 2 comment lines in the code, I get the expected behaviour.


REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import javafx.application.Application;
import javafx.application.Platform;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.layout.Pane;
import javafx.scene.layout.StackPane;
import javafx.scene.shape.Ellipse;
import javafx.stage.Stage;


public class Test1 extends Application {

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

    @Override
    public void start(Stage stage) throws Exception {       
        Pane topPane = new Pane();      
        Scene scene = new Scene(topPane, 600, 400);

        StackPane sp = new StackPane();
        Label l1 = new Label("1 2");
        Ellipse e1 = new Ellipse(100, 50);
        e1.setOpacity(0.5);
        sp.getChildren().addAll(l1, e1);            
        e1.radiusXProperty().bind(l1.widthProperty());
        e1.radiusYProperty().bind(l1.heightProperty());         
        topPane.getChildren().add(sp);
        sp.relocate(200, 100);

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

        Platform.runLater(() -> {
            //l1.setText("123");
            //l1.setText("1 2");
        });             

        stage.setScene(scene);
        stage.show();       
    }
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
Add sp.requestLayout(); after stage.Show();




Comments
Removing the 8-bp label as this bug does not meet the criteria for a backport.
09-04-2016

Changeset: 6ef9d5c0ad46 Author: ckyang Date: 2016-03-11 14:37 -0800 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/6ef9d5c0ad46 8137252: JavaFX StackPane bounds not updated Reviewed-by: kcr, jgiles
11-03-2016

+1
11-03-2016

Thanks for running the test and feedback. I have rewrote the test to be independent of resolution setting and system font size, plus addressed those comments suggested. http://cr.openjdk.java.net/~ckyang/JDK-8137252/webrev.03/
11-03-2016

I see why the test is failing. You are hard coding values that depend on the size of the system font. That is not going to be portable across different systems.
10-03-2016

Code changes look good to me. However the new unit test fails for me on Linux (assertion error due to mismatch between expected and actual bounds). Also, a couple of minor points: 1) The following comment in Parent::requestParentLayout(boolean) is not correct: + * propagate this force layout flag to its parent. 2) The comment is not grammatically correct (the verb tense is wrong) : + * It stores the reference to the current child being layout by its parent. laid out by its parent, maybe? 3) Maybe pick a better name than "testEmptyShapes" for the name of the test?
10-03-2016

Added comment as to why the earlier optimization fix in VirtualFlow.requestout() is commented. This is the only change made between webrev.01 and webrev.02. http://cr.openjdk.java.net/~ckyang/JDK-8137252/webrev.02/
10-03-2016

I can do some performance testing once this gets into the repo to see if there are any performance regressions - just ping me and I'll spend a an hour or two running my performance test suites.
23-02-2016

I believe the mystery to the lesser count value in TreeTableViewTest after the proposed fix is found. It is in VirtualFlow where it overrides its parent's requestLayout() method as part of an optimization effort. However the logic and comment of this overridden method looks puzzling to me. In a number of cases within VirtualFlow, we essentially calling setNeedsLayout() twice; making requestLayout() call an no-op in those cases. So what I did is to undo that optimization by making VirtualFlow.requestLayout() to simply call super.requestLayout() and that fixes the test problem. My evaluation is that this fix exposed an optimization bug (or assumption on a buggy layout implementation) in VirtualFlow. I propose we take in this fix and work on optimizing VirtualFlow separately, with a new JIRA, if there is a need. Here is my proposed fix: http://cr.openjdk.java.net/~ckyang/JDK-8137252/webrev.01/
23-02-2016

I can also confirm that all existing unit tests passed if I just undo the caching and resetting layoutFlag part of the fix. But without this part of the fix our fix is only a partial fix to this bug. I also found that the assertFalse(separator.isNeedsLayout()) statements in SeparatorSkinTest.java and VirtualFlowTest.java can be restored if we make another layout() call in their respective setup() method. The reason is that a Parent can be made "layout" dirty again by its children as it performs layout() traversing down its children requesting layout which then traverse up to parent!
18-02-2016

To follow up on your concern regarding "happening zero times in other places (in the unit tests).". Are we certain that those calls are really needed? The reason I ask is because I reduced the fix to address only the caching and clearing of layoutFlag and that alone reduces the count to zero: test_rt_35395_testCell_fixedCellSize: java.lang.AssertionError: expected:<2> but was:<0> and test_rt_35395_testCell_notFixedCellSize: java.lang.AssertionError: expected:<5> but was:<0> Here is the diff for the change I used: --- a/modules/graphics/src/main/java/javafx/scene/Parent.java +++ b/modules/graphics/src/main/java/javafx/scene/Parent.java @@ -1068,7 +1068,11 @@ * Calling this method while the Parent is doing layout is a no-op. */ public final void layout() { - switch(layoutFlag) { + // layoutFlag can be accessed or changed during layout processing. + // Hence we need to cache and reset it before performing layout. + LayoutFlags flag = layoutFlag; + setLayoutFlag(LayoutFlags.CLEAN); + switch(flag) { case CLEAN: break; case NEEDS_LAYOUT: @@ -1095,7 +1099,6 @@ ((SubScene)child).layoutPass(); } } - setLayoutFlag(LayoutFlags.CLEAN); performingLayout = false; break; }
17-02-2016

I agree this fix needs lots of testing. I have tested it with Ensemble8 and other programs in apps and toys, but it certainly wouldn't hurt to get more testing done. Do we have a layout or control benchmark suite somewhere?
17-02-2016

This webrev makes me a little nervous. I think there will need to be extra testing on the controls to make sure no regressions creep in. Also, I'm worried about some of the layouts happening more in places, and happening zero times in other places (in the unit tests). This means we might end up with extra layout happening and wasting cpu time, or the layout become stale when it shouldn't be.
17-02-2016

Attached 2 extracted unit test programs for visual verification.
17-02-2016

Kevin and Jonathan, Please review the proposed fix: http://cr.openjdk.java.net/~ckyang/JDK-8137252/webrev.00/ This fix addresses 2 issues in the bug: 1) The layoutFlag can be accessed or changed during layout processing. Hence we need to cache and reset it before performing layout. (see Parent.layout()) 2) The layout dirty optimization fails to consider the case where a layout request on its parent is required if that change isn't triggered by its parent. The fix has caused 8 unit test regressions in Control. I have verified that these tests were written to enforce existing layout implement logic instead of the expected result of the control specification. The proposed fix will do slightly more work than before we will need to fix the unit test. I will attached 2 extracted test programs that perform the same as the unit tests for visual verification. I also wrote a unit test, LayoutTest, to verify that the bug is fixed by the proposed fix.
17-02-2016

Attached a better test program that includes animation (Timeline) and run later as part of the test options.
17-02-2016

Also moving the binding lines after stage.show(); solves the problem.
20-01-2016

The program works fine if I commented out the following binding statements: e1.radiusXProperty().bind(l1.widthProperty()); e1.radiusYProperty().bind(l1.heightProperty()); and the print out of the various layoutBounds look good too: e1's layoutBounds = BoundingBox [minX:-100.0, minY:-50.0, minZ:0.0, width:200.0, height:100.0, depth:0.0, maxX:100.0, maxY:50.0, maxZ:0.0] l1's layoutBounds = BoundingBox [minX:0.0, minY:0.0, minZ:0.0, width:19.0, height:16.0, depth:0.0, maxX:19.0, maxY:16.0, maxZ:0.0] sp's layoutBounds = BoundingBox [minX:0.0, minY:0.0, minZ:0.0, width:202.0, height:102.0, depth:0.0, maxX:202.0, maxY:102.0, maxZ:0.0] The print out of the various layoutBounds with the binding statements are followed: e1's layoutBounds = BoundingBox [minX:-19.0, minY:-16.0, minZ:0.0, width:38.0, height:32.0, depth:0.0, maxX:19.0, maxY:16.0, maxZ:0.0] l1's layoutBounds = BoundingBox [minX:0.0, minY:0.0, minZ:0.0, width:19.0, height:16.0, depth:0.0, maxX:19.0, maxY:16.0, maxZ:0.0] sp's layoutBounds = BoundingBox [minX:0.0, minY:0.0, minZ:0.0, width:21.0, height:18.0, depth:0.0, maxX:21.0, maxY:18.0, maxZ:0.0] It is clear that the bounds of StackPane doesn't factor in e1's bounds which is a bug since e1 is one of its children.
03-11-2015