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

• 06: Full - Incremental (0b959273)
• 05: Full - Incremental (1d54e15f)
• 04: Full - Incremental (e9b49d47)
• 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.

[kevinrushforth]

I won't have time to do a detailed review for a while, but the updated approach to the fix looks promising. I note that the change in behavior will need to be documented another way, possibly in the base Window::sizeToScene method, since the newly added method is, correctly, not public (nor should it be).

When running the new test on macOS, I see a few test failures followed by a crash. The crash is clearly a bug in JavaFX glass code (I'll file a bug for that), but the failures point to a problem with the test.

I noticed while running it that there are no delays between various window operations in the tests. This will never work on Mac (and likely is a factor in provoking the crash), and will not be reliable on other platforms.

Here are the test failures:

SizeToSceneTest > testInitialSizeOnSizeToScene() FAILED
    org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77)
        at app//test.javafx.stage.SizeToSceneTest.testInitialSizeOnSizeToScene(SizeToSceneTest.java:197)

SizeToSceneTest > testInitialSizeSizeToSceneFullscreenOnOff() FAILED
    org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77)
        at app//test.javafx.stage.SizeToSceneTest.testInitialSizeSizeToSceneFullscreenOnOff(SizeToSceneTest.java:229)

SizeToSceneTest > testInitialSizeMaximizedOnOffSizeToScene() FAILED
    org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77)
        at app//test.javafx.stage.SizeToSceneTest.testInitialSizeMaximizedOnOffSizeToScene(SizeToSceneTest.java:245)

SizeToSceneTest > testInitialSizeFullscreenOnOffSizeToScene() FAILED
    org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77)
        at app//test.javafx.stage.SizeToSceneTest.testInitialSizeFullscreenOnOffSizeToScene(SizeToSceneTest.java:213)

SizeToSceneTest > testInitialSizeSizeToSceneMaximizedOnOff() FAILED
    org.opentest4j.AssertionFailedError: 1536.0 <= 410 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.javafx.stage.SizeToSceneTest.assertStageSceneBounds(SizeToSceneTest.java:77)
        at app//test.javafx.stage.SizeToSceneTest.testInitialSizeSizeToSceneMaximizedOnOff(SizeToSceneTest.java:261)

Java has been detached already, but someone is still trying to use it at -[GlassWindow(Overrides) windowDidResignKey:]:jfx/modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Overrides.m:96
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x0000000102f2fdbe, pid=25095, tid=259
#
# JRE version: Java(TM) SE Runtime Environment (21.0.2+13) (build 21.0.2+13-LTS-58)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (21.0.2+13-LTS-58, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-amd64)
# Problematic frame:
# C  [libglass.dylib+0x29dbe]  -[GlassWindow(Overrides) windowDidResignKey:]+0xde
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# jfx/tests/system/hs_err_pid25095.log
Gradle Test Executor 1 finished executing tests.
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#
0   libglass.dylib                      0x0000000102f2fd7b -[GlassWindow(Overrides) windowDidResignKey:] + 155
1   CoreFoundation                      0x00007ff807aea688 __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 137
2   CoreFoundation                      0x00007ff807b833fe ___CFXRegistrationPost_block_invoke + 88
3   CoreFoundation                      0x00007ff807b83353 _CFXRegistrationPost + 536
4   CoreFoundation                      0x00007ff807abd927 _CFXNotificationPost + 729
5   Foundation                          0x00007ff80892ab30 -[NSNotificationCenter postNotificationName:object:userInfo:] + 82
6   AppKit                              0x00007ff80ad2bd37 -[NSWindow resignKeyWindow] + 758
7   AppKit                              0x00007ff80b4d7e4b -[NSWindow _orderOut:calculatingKeyWithOptions:documentWindow:] + 326
8   AppKit                              0x00007ff80abdd8b7 NSPerformVisuallyAtomicChange + 132
9   AppKit                              0x00007ff80b4da0a7 -[NSWindow _reallyDoOrderWindowOutRelativeTo:] + 618
10  AppKit                              0x00007ff80b4da4ca -[NSWindow _reallyDoOrderWindow:] + 99
11  AppKit                              0x00007ff80b4da740 -[NSWindow _doOrderWindow:] + 295
12  AppKit                              0x00007ff80b4d5029 -[NSWindow _finishClosingWindow] + 306
13  AppKit                              0x00007ff80ae92a1d -[NSWindow _close] + 336
14  libglass.dylib                      0x0000000102f3071f -[GlassWindow_Normal close] + 79
15  Foundation                          0x00007ff8089a0743 __NSThreadPerformPerform + 177
16  CoreFoundation                      0x00007ff807af4eba __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
17  CoreFoundation                      0x00007ff807af4e5c __CFRunLoopDoSource0 + 157
18  CoreFoundation                      0x00007ff807af4c93 __CFRunLoopDoSources0 + 311
19  CoreFoundation                      0x00007ff807af38bf __CFRunLoopRun + 916
20  CoreFoundation                      0x00007ff807af2ec1 CFRunLoopRunSpecific + 560
21  libjli.dylib                        0x000000010201bee2 CreateExecutionEnvironment + 386
22  libjli.dylib                        0x00000001020178bd JLI_Launch + 1357
23  java                                0x0000000101faec17 main + 391
24  dyld                                0x00007ff8076be41f start + 1903