Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271054: [REDO] Wrong stage gets focused after modal stage creation #598

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

@tsayao
Copy link
Collaborator

@tsayao tsayao commented Aug 5, 2021

Found the problem thru this path:

WindowStage.java

    final void handleFocusDisabled() {
        if (activeWindows.isEmpty()) {
            return;
        }
        WindowStage window = activeWindows.get(activeWindows.size() - 1);
        window.setIconified(false);
        window.requestToFront();
        window.requestFocus();
    }

glass_window.cpp

void WindowContextBase::process_focus(GdkEventFocus* event) {
   ...

    if (jwindow) {
        if (!event->in || isEnabled()) {
            mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocus,
                    event->in ? com_sun_glass_events_WindowEvent_FOCUS_GAINED : com_sun_glass_events_WindowEvent_FOCUS_LOST);
            CHECK_JNI_EXCEPTION(mainEnv)
        } else {
            mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocusDisabled);
            CHECK_JNI_EXCEPTION(mainEnv)
        }
    }
}

So glass_window.cpp was triggering jWindowNotifyFocusDisabled which triggered the java code on the Primary Stage (on the bug reproduce code).

The docs states:

    /**
     * Enables or disables the window.
     *
     * A disabled window is unfocusable by definition.
     * Also, key or mouse events aren't generated for disabled windows.
     *
     * When a user tries to activate a disabled window, or the window gets
     * accidentally brought to the top of the stacking order, the window
     * generates the FOCUS_DISABLED window event. A Glass client should react
     * to this event and bring the currently active modal blocker of the
     * disabled window to top by calling blocker's minimize(false), toFront(),
     * and requestFocus() methods. It may also 'blink' the blocker window to
     * further attract user's attention.
     *
     .....

So I guess the C++ side looks ok, java side on handleFocusDisabled I did not understand why it requestToFront and requestFocus on the latest window.

The solution makes disabled windows unfocusable and prevents mouse and keyboard events as the docs states, making it compatible with other platforms.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271054: [REDO] Wrong stage gets focused after modal stage creation

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/598/head:pull/598
$ git checkout pull/598

Update a local copy of the PR:
$ git checkout pull/598
$ git pull https://git.openjdk.java.net/jfx pull/598/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 598

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/598.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 5, 2021

馃憢 Welcome back tsayao! 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.

@openjdk openjdk bot added the rfr label Aug 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 5, 2021

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Aug 6, 2021

image

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Aug 9, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@pankaj-bansal
Copy link
Collaborator

@pankaj-bansal pankaj-bansal commented Aug 15, 2021

I ran the testcase attached in JBS with the fix multiple times. I can still reproduce the issue on Ubuntu 20.0 almost always, so the fix does not seem to work for me.

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Aug 15, 2021

Weird, It works consistently for me on 20.04. Just tested again to be sure.

@pankaj-bansal
Copy link
Collaborator

@pankaj-bansal pankaj-bansal commented Aug 16, 2021

Weird, It works consistently for me on 20.04. Just tested again to be sure.

I am running a 20.04 VM. The test fails for me 60-70% of the time. I will request someone in team to try this once.

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Sep 12, 2021

@pankaj-bansal Could you please re-test to confirm that it does not work on your VM?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 17, 2021

I just tested this on my Ubuntu 20.04 VM and the problem still reproduces for me with or without this fix.

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Sep 21, 2021

Docs states:

        /*
         * When a user tries to activate a disabled window, or the window gets
         * accidentally brought to the top of the stacking order, the window
         * generates the FOCUS_DISABLED window event. A Glass client should react
         * to this event and bring the currently active modal blocker of the
         * disabled window to top by calling blocker's minimize(false), toFront(),
         * and requestFocus() methods. It may also 'blink' the blocker window to
         * further attract user's attention.
         */

But WindowStage.java actually does:

    final void handleFocusDisabled() {
        if (activeWindows.isEmpty()) {
            return;
        }
        WindowStage window = activeWindows.get(activeWindows.size() - 1);
        window.setIconified(false);
        window.requestToFront();
        window.requestFocus();
    }

It brings up current activeWindows.size() - 1, not actually the MODAL blocker. Seems wrong to me.

The event is born on glass_window.cpp:

mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocusDisabled);

The current proposed code (which I will redo) blocks this line from being sent (on non-virtual machines apparently). But commenting the jWindowNotifyFocusDisabled should make the problem disappear (just to test the theory).

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Sep 21, 2021

I have proposed a different fix, but it might have concurrency issues and probably others that should be investigated.

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Sep 30, 2021

Just a note, our javafx application at the company I work for is unusable with javafx17 on Linux, windows go behind their owner or pops up wrongly. Can't use 17 line until it's fixed.

Copy link
Collaborator

@pankaj-bansal pankaj-bansal left a comment

I tested this using the manual test and looks ok to me. I ran the system tests and it is not introducing any regression.

@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Oct 6, 2021

@pankaj-bansal Can you try the test attached to the JBS on macos and windows? Thanks.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 12, 2021

This fix reintroduced JDK-8240640 on macOS. With the patch applied I get the following failure:

test.robot.javafx.stage.WrongStageFocusWithApplicationModalityTest > testWindowFocusByClosingAlerts FAILED
    java.lang.AssertionError: Timeout: Alerts not closed, wrong focus
        at org.junit.Assert.fail(Assert.java:91)
        at org.junit.Assert.assertTrue(Assert.java:43)
        at test.robot.javafx.stage.WrongStageFocusWithApplicationModalityTest.waitForLatch(WrongStageFocusWithApplicationModalityTest.java:82)
        at test.robot.javafx.stage.WrongStageFocusWithApplicationModalityTest.testWindowFocusByClosingAlerts(WrongStageFocusWithApplicationModalityTest.java:70)

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The above test fails for me on Linux (Ubuntu 20.04 VM) as well. The manual test case attached to JDK-8269429 also fails on Linux. I think you will need a solution that doesn't modify the logic in WindowStage.java.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2021

鈿狅笍 @tsayao This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@tsayao tsayao changed the title 8271054: [REDO] Wrong stage gets focused after modal stage creation WIP: 8271054: [REDO] Wrong stage gets focused after modal stage creation Oct 29, 2021
@openjdk openjdk bot removed the rfr label Oct 29, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Nov 1, 2021

The problem occurs because WindowStage.activeWindows gets out of order, because of focus events. So when "Focus Disabled" happens it brings up the wrong window.

Probably because "Focus Stealing" situation which is mentioned on:
https://docs.gtk.org/gtk3/method.Window.present_with_time.html

I'm working on the fix.

@tsayao tsayao changed the title WIP: 8271054: [REDO] Wrong stage gets focused after modal stage creation 8271054: [REDO] Wrong stage gets focused after modal stage creation Nov 1, 2021
@openjdk openjdk bot added the rfr label Nov 1, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Nov 1, 2021

The ideal fix would be to know the original event and save it's X11 serial event number to pass to gtk_window_present_with_time so "focus stealing" would not happen. But I don't think it's possible because it should save the "event serial number" on the original mouse click to open a window (for example). As you have thought, many "original event" combinations are possible.

The fix just reorders the WindowStage.activeWindows by calling FOCUS_GAINED. So when FOCUS_DISABLED happens it focuses the correct window. Please not that with this fix there are extra calls to FOCUS events and reorder of WindowStage.activeWindows (although they are not expensive).

@kevinrushforth kevinrushforth self-requested a review Nov 2, 2021
@tsayao
Copy link
Collaborator Author

@tsayao tsayao commented Nov 30, 2021

It would be nice to have this fix on jfx 17, since currently the issue causes random windows to be focused and it's very confusing to the end user.

@pankaj-bansal
Copy link
Collaborator

@pankaj-bansal pankaj-bansal commented Dec 4, 2021

I ran the full systemTest and this test is not introducing any regression. Also, the manual test attached in JBS works fine with this change.

@@ -445,6 +445,7 @@ static void process_events(GdkEvent* event, gpointer data)
}

if (ctx != NULL) {

Copy link
Collaborator

@pankaj-bansal pankaj-bansal Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably remove this unnecessary change as this is only one change in the file.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 11, 2021

On my Ubuntu 20.04 VM the bug still happens about 1/2 the time with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants