8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window

[Maran23]

This PR fixes the problem that maximizing/fullscreen a Stage or Dialog is broken when sizeToScene() was called before or after.

The approach here is to ignore the sizeToScene() request when the Stage is maximized or set to fullscreen.
Otherwise the Window Manager of the OS will be confused and you will get weird flickering or wrong Window buttons (e.g. on Windows, the 'Maximize' button still shows the 'Restore' Icon, while we already resized the Stage to not be maximized).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion jfx23 to be approved (needs to be created)
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window (Bug - P4)

Reviewers

• Lukasz Kostyra (@lukostyra - Committer) 🔄 Re-review required (review applies to e58a2fda)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1382

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mhanl! 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.

[mlbridge]

Webrevs

• 03: Full - Incremental (eeab0fa8)
• 02: Full - Incremental (e58a2fda)
• 01: Full - Incremental (bbdb4db7)
• 00: Full (bc607409)

[kevinrushforth]

Since this involved changing the specified behavior it will need a CSR. If we agree that this is the right behavior, then the CSR will be trivial.

/csr
/reviewers 2

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@Maran23 please create a CSR request for issue JDK-8326619 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[openjdk]

@kevinrushforth
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).

[kevinrushforth]

This seems like a reasonable fix and spec change. Have you tested the case of calling sizeToScene before setting full-screen or maximzed? Since the pending flag will still be set in that case, I want to make sure that case is tested as well.

Also, if this fixed JDK-8316425, then that bug should be closed as a duplicate of this one.

@lukostyra @arapte can you also review this?

[kevinrushforth]

Btw, I get the following test failures on our headful Linux test systems:

SizeToSceneFullscreenTest > testInitialSizeFullscreen FAILED
    java.lang.AssertionError: Stage height expected:<1080.0> but was:<1117.0>
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:555)
        at test.javafx.stage.SizeToSceneFullscreenTest.testInitialSizeFullscreen(SizeToSceneFullscreenTest.java:78)

This seems unrelated to your fix. I think the problem might be that in full-screen mode the stage can be bigger than the visible screen size or maybe the decorations are just larger on that system. You might either need to increase the tolerance values or instead check for >= visible width / height (possibly with some small tolerance).

[Maran23]

This seems like a reasonable fix and spec change. Have you tested the case of calling sizeToScene before setting full-screen or maximzed? Since the pending flag will still be set in that case, I want to make sure that case is tested as well.

I thought about this as well but could not find any problem at least on Windows.
If we want to be perfectly safe, we may still want to set the flag when sizeToScene() is called. What do you think?

I used the following code to test this, and it didn't matter when sizeToScene() was called:

   @Override
    public void start(Stage primaryStage) throws Exception {
        StackPane root = new StackPane();
        Button wss = new Button("Wss");
        wss.setPrefSize(50, 50);
        root.getChildren().add(wss);

        Scene scene = new Scene(root);

        Stage stage = new Stage();
        stage.setWidth(500);
        stage.setMaximized(true);
        stage.sizeToScene();
        stage.setScene(scene);
        stage.show();
    }

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[lukostyra]

LGTM, tests also seem to be fine on my end (checked on Windows and Ubuntu 22.04 LTS)

[bridgekeeper]

@Maran23 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[kevinrushforth]

The fix looks good. The spec changes (updated javadocs) look good. Can you create the CSR for the spec change?

I have a couple overall comments:

• I wanted to verify different orders of operation, so I wrote a (manual) test program and attached it to the JBS bug. It covers the following cases:
  • set ; sizeToScene ; show
  • sizeToScene ; set ; show
  • show ; set ; sizeToScene
  • show ; sizeToScene ; set

I verified that the first 3 are broken today. All cases work with your fix. I think it might be a good idea to add automated tests for the different orderings.

• Please merge the latest master. Note that the calls to Util.shutdown in the tests must be fixed after this is done or they will no longer compile.

[kevinrushforth]

@Maran23 I think this is pretty close to being ready to go in. At a minimum, you will need to merge master and then fix the test so that it will compile, and then create a CSR with the updated specification (I can help with that if needed). My only other suggestion was around additional tests that might be useful, but they could be done as a follow-on fix.

[Maran23]

@Maran23 I think this is pretty close to being ready to go in. At a minimum, you will need to merge master and then fix the test so that it will compile, and then create a CSR with the updated specification (I can help with that if needed). My only other suggestion was around additional tests that might be useful, but they could be done as a follow-on fix.

Yes, sure, I've just been very busy with my day job over the last few weeks. I hopefully have more time now though :)
And I totally agree with writing more tests, always good to have and to ensure quality. So no need for a follow-on ticket.

[Maran23]

• I wanted to verify different orders of operation, so I wrote a (manual) test program

I'm retesting and writing tests right now and reproduced one usecase out of my head that indeed 'fails' now.
Take the following code:

        Button button = new Button();
        button.setMinSize(440, 440);

        Scene scene = new Scene(button);
        stage.setTitle("JavaFX test stage!");
        stage.setScene(scene);

        stage.setWidth(50);
        stage.setHeight(50);

        stage.setFullScreen(true);
        stage.sizeToScene();
        stage.setFullScreen(false);

        stage.show();

With my logic, the sizeToScene() flag is not remembered, so the scene is not adjusted in the sizeToScene style after I 'go out' of fullscreen mode.

If I do instead:

        stage.sizeToScene();
        stage.setFullScreen(true);
        stage.setFullScreen(false);

The flag is remembered and the scene has the size of the button. Not sure what the expectation is here, but we could fix this problem by still remembering the flag if called.