jfx icon indicating copy to clipboard operation
jfx copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

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

Open Maran23 opened this issue 1 year ago • 18 comments
trafficstars

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

  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
  • [x] Change requires CSR request JDK-8334966 to be approved

Issues

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

Reviewers

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

Maran23 avatar Feb 26 '24 20:02 Maran23

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

bridgekeeper[bot] avatar Feb 26 '24 20:02 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Feb 26 '24 20:02 mlbridge[bot]

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

kevinrushforth avatar Feb 27 '24 00:02 kevinrushforth

@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[bot] avatar Feb 27 '24 00:02 openjdk[bot]

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

openjdk[bot] avatar Feb 27 '24 00:02 openjdk[bot]

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 avatar Feb 28 '24 23:02 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).

kevinrushforth avatar Feb 29 '24 00:02 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.

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();
    }

Maran23 avatar Mar 08 '24 16:03 Maran23

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: lkostyra, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

  • 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data
  • ca04c87d307c36591162af8cd6298ede17812834: 8334713: WebKit build failed on LoongArch64 because currentStackPointer is undefined
  • 0d2a0a462e500b9bb7ac4fc4f45beff9a378ea60: 8334161: Enable -Werror for javac tasks to fail on any warnings
  • b80b106df9eee12fc42d85cf9386975dc56c5a57: 8314754: Minor ticks are not getting updated both the axes in LineChart
  • d9c88630fad0e9cd4a93ac0901141f4ae7939be0: 8311124: [Windows] User installed font 8281327 fix does not work for all cases
  • 101e5175ff429828de25204bc3e5f216d383060b: 8334657: Enable binary check
  • 83c8dcc91a892c557c5b40064e94274caa89ce6f: 8327255: javac lint warnings: removal, missing-explicit-ctor
  • e9b10ab816c2bf7b3bb88bf6c9c0ccb826bff34c: 8334731: GHA: build on macOS / aarch64
  • 17c2dba0d5ab7608e7df6673c67ace7212a60040: 8334739: XYChart and (Stacked)AreaChart properties return incorrect beans
  • c0216ae94862c860e238b251a5e51aa7c83b9a1d: 8088923: IOOBE when adding duplicate categories to the BarChart
  • ... and 12 more: https://git.openjdk.org/jfx/compare/94aa2b68d01af6096a18316ab36c911fe8cec572...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

@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!

bridgekeeper[bot] avatar Apr 30 '24 18:04 bridgekeeper[bot]

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

kevinrushforth avatar May 20 '24 21:05 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.

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 avatar May 23 '24 12:05 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.

Maran23 avatar May 23 '24 13:05 Maran23

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

kevinrushforth avatar May 24 '24 18:05 kevinrushforth

@Maran23 Can you create the CSR? The updated docs (with the small suggestion I made) look ready.

kevinrushforth avatar Jun 05 '24 22:06 kevinrushforth

@lukostyra Please re-review.

kevinrushforth avatar Jun 26 '24 20:06 kevinrushforth

Based on those failures it would be a good idea to add tolerance to the check for >= visual bounds.

kevinrushforth avatar Jun 27 '24 11:06 kevinrushforth

Change looks good, but I'm still seeing failures on my macOS system (Sonoma 14.5, MacBook Pro M1 Max):

~I'm confused, the screen height is reported as 1085? ...~

Search the web led me to this: Browser size for Mac: 1920 -> 1903 | 1200 -> 1085

So it looks like JavaFX is using 1080 for whatever reason while the visible bounds are 1085. Weird, but my next push will add a delta now.

Maran23 avatar Jun 27 '24 11:06 Maran23

@Maran23 This is ready for you to integrate now.

kevinrushforth avatar Jul 02 '24 21:07 kevinrushforth

/integrate

Maran23 avatar Jul 03 '24 09:07 Maran23

Going to push as commit 5656b80f8a584a2d4d791f9fab83f1719b29f986. Since your change was applied there have been 23 commits pushed to the master branch:

  • 6c3fb5f557597a5a226d4a7bd41d84b7feb4a162: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
  • 1fb8755dc6452bdb07685c02272ecfc578fb1eba: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data
  • ca04c87d307c36591162af8cd6298ede17812834: 8334713: WebKit build failed on LoongArch64 because currentStackPointer is undefined
  • 0d2a0a462e500b9bb7ac4fc4f45beff9a378ea60: 8334161: Enable -Werror for javac tasks to fail on any warnings
  • b80b106df9eee12fc42d85cf9386975dc56c5a57: 8314754: Minor ticks are not getting updated both the axes in LineChart
  • d9c88630fad0e9cd4a93ac0901141f4ae7939be0: 8311124: [Windows] User installed font 8281327 fix does not work for all cases
  • 101e5175ff429828de25204bc3e5f216d383060b: 8334657: Enable binary check
  • 83c8dcc91a892c557c5b40064e94274caa89ce6f: 8327255: javac lint warnings: removal, missing-explicit-ctor
  • e9b10ab816c2bf7b3bb88bf6c9c0ccb826bff34c: 8334731: GHA: build on macOS / aarch64
  • 17c2dba0d5ab7608e7df6673c67ace7212a60040: 8334739: XYChart and (Stacked)AreaChart properties return incorrect beans
  • ... and 13 more: https://git.openjdk.org/jfx/compare/94aa2b68d01af6096a18316ab36c911fe8cec572...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jul 03 '24 09:07 openjdk[bot]

@Maran23 Pushed as commit 5656b80f8a584a2d4d791f9fab83f1719b29f986.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar Jul 03 '24 09:07 openjdk[bot]

I'm not sure how we missed this in testing, but the full screen tests fail on Ubuntu 22.04 because a full screen window can be larger than the visual bounds. I started seeing nightly headful test failures after this was integrated, but didn't have time to look at it until now. I filed JDK-8336272 to fix the test.

kevinrushforth avatar Jul 11 '24 22:07 kevinrushforth