jfx icon indicating copy to clipboard operation
jfx copied to clipboard

8359601: Fix window button states of an extended stage

Open mstr2 opened this issue 6 months ago • 12 comments

The window button states (disabled/hidden) of extended stages with a HeaderButtonOverlay or custom header buttons are inconsistent with what we would expect from the OS (Windows and Linux). To figure out what we would expect, I started with gathering some data. The following table shows the button states of system-decorated windows on various platforms:

Windows 11

Window attributes Iconify Maximize Close
resizable, not modal visible visible visible
not resizable, not modal visible visible, disabled visible
resizable, modal visible, disabled visible visible
not resizable, modal hidden hidden visible, utility-style

Ubuntu 24 / Fedora 41 (GNOME)

Window attributes Iconify Maximize Close
resizable, not modal visible visible visible
not resizable, not modal visible hidden visible
resizable, modal visible, not working visible, not working visible
not resizable, modal visible, not working hidden visible

Kubuntu 24 (KDE)

Window attributes Iconify Maximize Close
resizable, not modal visible visible visible
not resizable, not modal visible hidden visible
resizable, modal visible, not working visible visible
not resizable, modal visible, not working hidden visible

Observations

  1. On Windows, buttons are generally disabled when their operation is not possible with the given window attributes.
    • Exception: modal/non-resizable windows look like utility windows (iconify and maximize are hidden)
  2. On GNOME and KDE, buttons are generally hidden when their operation is not possible.
    • Exception: iconify and maximize on modal windows is not hidden, but seems to simply not do anything (bug?)

Permitted window button operations

Given the gathered observations and some simple logic, this is the table of operations that are permitted for all combinations of modal and resizable window attributes:

Window attributes Iconify Maximize Close
resizable, not modal yes yes yes
not resizable, not modal yes no yes
resizable, modal no yes yes
not resizable, modal no no yes

Fixes

This PR includes the following changes:

  1. Unused code relating to window modality is removed.
  2. The disabled states of HeaderButtonOverlay as well as HeaderButtonBehavior are changed to match the table above.
  3. The stylesheets for GNOME and KDE are changed such that disabled buttons are hidden.
  4. The stylesheet for Windows is changed such that a modal/non-resizable window looks like a utility window.

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)

Issue

  • JDK-8359601: Fix window button states of an extended stage (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1831

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

Using diff file

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

Using Webrev

Link to Webrev Comment

mstr2 avatar Jun 15 '25 20:06 mstr2

/reviewers 2

mstr2 avatar Jun 15 '25 20:06 mstr2

:wave: Welcome back mstrauss! 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 Jun 15 '25 20:06 bridgekeeper[bot]

@mstr2 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:

8359601: Fix window button states of an extended stage
8359763: Close request handler is not called for an extended stage

Reviewed-by: mmack, 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 27 new commits pushed to the master branch:

  • dbd43a371a2460c3cf8aed108d349b301bd2a1b2: 8363813: Missing null check in GlassScreen
  • 7b59ebcec74cf3e0da25e35b22a9722b9d93ebdb: 8362873: Regression in BorderPane after JDK-8350149
  • da6965bea005bbaf0bcfd05e295bbab4ea4f5eaa: 8362079: Change JavaFX release version to 26
  • ... and 24 more: https://git.openjdk.org/jfx/compare/fd30c94893156644c0d803b3e7fd8c9731d65fe6...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 Jun 15 '25 20:06 openjdk[bot]

@mstr2 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 Jun 15 '25 20:06 openjdk[bot]

Most of this seems to be working well!

But I have found a likely bug. Hopefully fairly self-explanatory from the code.

Minimise icon is normally disabled, but still enabled when EXTENDED is used & when dialog is resizable.

Just flip comment on dialog.initStyle(StageStyle.EXTENDED) to see difference.

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonType;
import javafx.scene.control.Dialog;
import javafx.scene.layout.StackPane;
import javafx.stage.Modality;
import javafx.stage.Stage;
import javafx.stage.StageStyle;

public class MinimiseIconEnabledBug extends Application {
    @Override
    public void start(Stage primaryStage) {
        Button button = new Button("Click");
        button.setOnAction(e -> {
            final Dialog<Object> dialog = new Dialog<>();
            dialog.initOwner(primaryStage);
            // Existing behaviour: minimise icon disabled when commented
            // Possible "bug": Minimise icon enabled when uncommented
            // dialog.initStyle(StageStyle.EXTENDED);
            dialog.initModality(Modality.NONE);
            dialog.setResizable(true); // This is important
            dialog.setTitle("My Dialog");
            dialog.setContentText("Lorem ipsum dolor sit amet, consectetur adipiscing...");
            dialog.getDialogPane().getButtonTypes().addAll(ButtonType.OK);
            dialog.show();
        });

        StackPane root = new StackPane(button);
        Scene scene = new Scene(root, 300, 200);

        primaryStage.setTitle("My First JavaFX App");
        primaryStage.setScene(scene);
        primaryStage.show();
    }

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

credmond avatar Jun 16 '25 23:06 credmond

Another suspected issue (not really related to this PR), close request handlers are not called when using EXTENDED. E.g.:

        stage.setOnCloseRequest(event -> {
            System.out.println("Never called...");;
        });

Same for dialogs...

credmond avatar Jun 17 '25 00:06 credmond

Good catch, a non-modal owned window also can't be iconified. I've added code and tests for this scenario.

mstr2 avatar Jun 17 '25 00:06 mstr2

Another suspected issue (not really related to this PR), close request handlers are not called when using EXTENDED. E.g.:

        stage.setOnCloseRequest(event -> {
            System.out.println("Never called...");;
        });

Same for dialogs...

Sample:

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonType;
import javafx.scene.control.Dialog;
import javafx.scene.layout.StackPane;
import javafx.stage.Modality;
import javafx.stage.Stage;
import javafx.stage.StageStyle;

public class MinimiseIconEnabledBug extends Application {
    @Override
    public void start(Stage primaryStage) {
        Button button = new Button("Click");
        button.setOnAction(e -> {
            final Dialog<Object> dialog = new Dialog<>();
            dialog.initOwner(primaryStage);
            // dialog.initStyle(StageStyle.EXTENDED); // uncomment for bug
            dialog.initModality(Modality.NONE);
            dialog.setResizable(true); // This is important
            dialog.getDialogPane().getButtonTypes().addAll(ButtonType.OK);
            dialog.setOnCloseRequest(dialogEvent -> {
                System.out.println("Close the dialog pane. This is not called when EXTENDED is used.");
            });
            dialog.show();
        });

        StackPane root = new StackPane(button);
        Scene scene = new Scene(root, 300, 200);
        primaryStage.setScene(scene);
        primaryStage.show();
    }

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

credmond avatar Jun 17 '25 00:06 credmond

I've filed another ticket for that: JDK-8359763.

mstr2 avatar Jun 17 '25 00:06 mstr2

Good catch, a non-modal owned window also can't be iconified. I've added code and tests for this scenario.

Looks good, can't find anymore issues...

credmond avatar Jun 17 '25 01:06 credmond

JavaFX modal windows are not native modals, as evidenced by the code removal on this PR (it's not used) . On GNOME, native modal windows automatically remove the minimize and maximize buttons.

image

I did an experiment and found that it's possible to get native modal windows (at least with GTK Glass) by passing the modality flag from WindowStage to createWindow. I haven’t done extensive testing, but it seems to work - likely because the modality can’t be changed afterward.

tsayao avatar Jun 19 '25 00:06 tsayao

JavaFX modal windows are not native modals, as evidenced by the code removal on this PR (it's not used) . On GNOME, native modal windows automatically remove the minimize and maximize buttons.

I did an experiment and found that it's possible to get native modal windows (at least with GTK Glass) by passing the modality flag from WindowStage to createWindow. I haven’t done extensive testing, but it seems to work - likely because the modality can’t be changed afterward.

That's a good idea for an enhancement.

mstr2 avatar Jun 19 '25 17:06 mstr2

I've filed another ticket for that: JDK-8359763.

Just curious, why a separate bug for this instead of fixing in this PR and avoiding the headaches/conversations of more PRs -- is it more complicated that it sounds?

credmond avatar Jun 21 '25 21:06 credmond

I've filed another ticket for that: JDK-8359763.

Just curious, why a separate bug for this instead of fixing in this PR and avoiding the headaches/conversations of more PRs -- is it more complicated that it sounds?

Shorter PRs tend to get more timely reviews. That being said, since it is closely related, I've added the fix to this PR.

/issue add JDK-8359763

mstr2 avatar Jun 21 '25 21:06 mstr2

@mstr2 Adding additional issue to issue list: 8359763: Close request handler is not called for an extended stage.

openjdk[bot] avatar Jun 21 '25 21:06 openjdk[bot]

I also checked the behavior in macOS Sequoia 15.5. There, a window where setResizable(false) is called before showing the stage, unexpectedly has a working maximize button even without StageStyle.EXTENDED. Looks like this was not the case prior to the integration of #1605, so this behavior was probably introduced there and may need a fix in this PR or elsewhere.

The implementation relied on the _setResizable JNI method being called in a very particular way that has since changed (internally, it would call another method with the same name, but the implementation would actually toggle the resizable state, not set it to a given value). I've fixed this problem.

mstr2 avatar Jun 21 '25 23:06 mstr2

I have found another bug/issue, which is again related to wrapping a root node in a StackPane (something that is common in, for example, ControlsFx). Although the observed behaviour is probably strictly logically correct, I think this should be prevented (e.g, HeaderBar has/gets special meaning/treatment).

public class DisappearingHeaderBarNodesBug extends Application {

    @Override
    public void start(Stage primaryStage) {
        BorderPane root = new BorderPane();
        Button button = new Button("Click for \"bug()\"!");
        button.setOnAction(_ -> bug(root));
        root.setCenter(new VBox(new Label("Something above"), button, new Label("Something below")));
        root.setTop(getHeaderBar());

        primaryStage.initStyle(StageStyle.EXTENDED);
        primaryStage.setScene(new Scene(root, 300, 200));
        primaryStage.show();
    }

    // Wrapping the root node in a stackpane is common
    private void bug(Pane root) {
        StackPane stackPane = new StackPane();
        root.getScene().setRoot(stackPane);
        stackPane.getChildren().addFirst(root);
    }

    private HeaderBar getHeaderBar() {
        HeaderBar headerBar = new HeaderBar();
        headerBar.setCenter(new Label("!!! HeaderBar !!!"));
        headerBar.setLeading(new Label("L"));
        return headerBar;
    }

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

As you can see, after you click the button, and resize, the HeaderBar can disappear completely:

https://github.com/user-attachments/assets/93a320a5-0010-4941-a2a4-9e0e65cbd57d

credmond avatar Jun 24 '25 08:06 credmond

@credmond We should keep this PR focused, let's use the mailing list or JBS to discuss other issues.

mstr2 avatar Jun 24 '25 15:06 mstr2

FYI to any reviewer: I've been using this branch's EXTENDED/HeaderBar extensively now without any new issues...

credmond avatar Jul 07 '25 23:07 credmond

I think this fix should make the 25 release if possible, as it would be unfortunate to preview the new feature with distracting bugs included. The fix is also quite well-tested at this point.

@kevinrushforth @andy-goryachev-oracle Do you have any review capacity to spare for this within the current RDP?

mstr2 avatar Jul 22 '25 18:07 mstr2

I think this fix should make the 25 release if possible, as it would be unfortunate to preview the new feature with distracting bugs included. The fix is also quite well-tested at this point.

@kevinrushforth @andy-goryachev-oracle Do you have any review capacity to spare for this within the current RDP?

Agreed. It wouldn't take long until others come across this and raise the same issue.

credmond avatar Jul 22 '25 19:07 credmond

I think this fix should make the 25 release if possible, as it would be unfortunate to preview the new feature with distracting bugs included. The fix is also quite well-tested at this point.

@kevinrushforth @andy-goryachev-oracle Do you have any review capacity to spare for this within the current RDP?

Yes, I'll review it this week. It seems a good candidate for backporting to jfx25.

kevinrushforth avatar Jul 22 '25 19:07 kevinrushforth

The headful CI tests passed on Linux and macOS (I will need to manually run it on Windows).

kevinrushforth avatar Jul 24 '25 22:07 kevinrushforth

Headful tests passed on Windows.

kevinrushforth avatar Jul 25 '25 12:07 kevinrushforth

/integrate

mstr2 avatar Jul 25 '25 13:07 mstr2

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

  • dbd43a371a2460c3cf8aed108d349b301bd2a1b2: 8363813: Missing null check in GlassScreen
  • 7b59ebcec74cf3e0da25e35b22a9722b9d93ebdb: 8362873: Regression in BorderPane after JDK-8350149
  • da6965bea005bbaf0bcfd05e295bbab4ea4f5eaa: 8362079: Change JavaFX release version to 26
  • ... and 24 more: https://git.openjdk.org/jfx/compare/fd30c94893156644c0d803b3e7fd8c9731d65fe6...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Jul 25 '25 13:07 openjdk[bot]

@mstr2 Pushed as commit bc433da812461a1c2796cdb3123f814e4ce532d5.

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

openjdk[bot] avatar Jul 25 '25 13:07 openjdk[bot]

/backport jfx jfx25

mstr2 avatar Jul 25 '25 13:07 mstr2

@mstr2 the backport was successfully created on the branch backport-mstr2-bc433da8-jfx25 in my personal fork of openjdk/jfx. To create a pull request with this backport targeting openjdk/jfx:jfx25, just click the following link:

:arrow_right: Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit bc433da8 from the openjdk/jfx repository.

The commit being backported was authored by Michael Strauß on 25 Jul 2025 and was reviewed by Markus Mack and Kevin Rushforth.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jfx:

$ git fetch https://github.com/openjdk-bots/jfx.git backport-mstr2-bc433da8-jfx25:backport-mstr2-bc433da8-jfx25
$ git checkout backport-mstr2-bc433da8-jfx25
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jfx.git backport-mstr2-bc433da8-jfx25

openjdk[bot] avatar Jul 25 '25 13:07 openjdk[bot]