8284654: Modal behavior returns to wrong stage

[tsayao]

When there's an APPLICATION_MODAL window, all other windows are disabled and re-enabled when the APPLICATION_MODAL window closes. This causes requestToFront() to be called on every window, and it does not guarantee order.

The fix also works for:
https://bugs.openjdk.java.net/browse/JDK-8269429

But this one might need another fix:
https://bugs.openjdk.java.net/browse/JDK-8240640

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8284654: Modal behavior returns to wrong stage

Reviewers

• Ambarish Rapte (@arapte - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 784

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

Using diff file

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

[bridgekeeper]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (a6cad45b)
• 01: Full - Incremental (b024de58)
• 00: Full (72317b21)

[kevinrushforth]

I'll take a look at it, and also ask @arapte to review. As you know from my comment in the bug report, I especially want to ensure that this causes no regressions, so need to test JDK-8240640 on macOS.

/reviewers 2

[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 1 of role reviewers, 1 of role authors).

[arapte]

This change fixes the exact issue that is reported in JDK-8284654.
and JDK-8240640 does not reproduce with this change.

But I observed another issue that occurs with this change.
Steps:

1. Run the sample attached to JDK-8284654.
2. Click button One and Two both once
3. Re-arrange both the windows such that they are visible.
4. Click the button "Click here" in Window Two: Error dialog will be displayed
5. Click anywhere in Window One
6. Close the Error dialog
   => Observe that Window One gets the focus and not Window Two.

I think the expected behavior here should be: After closing a dialog the Focus should return to its parent window.

[tsayao]

@arapte I think it's another bug on Linux, as the window should not accept focus when disabled, so it should not reorder windows when you click it ("Window One" on step 5).

Did you test it on Linux?

[tsayao]

Step 5 falls on:

    mainEnv->CallVoidMethod(jwindow, jWindowNotifyFocusDisabled);

which is correct, so might be a bug on java code.

[arapte]

Did you test it on Linux?

I tested it on MacOS Catalina 10.15.7

the window should not accept focus when disabled,

The window does not really gets the focus. The button cannot be clicked. but it does bring the window in foreground.

Yes, It may be a different bug. The similarity is that the focus does not go back to parent stage after closing the dialog.

[tsayao]

The test does not set the Window owner. It's one APPLICATION_MODAL window.

I have adjusted the PR to handle this case. It will set focus on the latest opened window when re-enabling, but will not change stacking order.

[arapte]

The original fix gets affected by the new commit.
Now, after closing dialog the focus always returns only to the stage that was created later.

1. Run the sample attached to JDK-8284654.
2. Click button One and Two both once in order
3. Re-arrange both the windows such that they are visible.
4. Click the button "Click here" in Window One: Error dialog will be displayed
5. Close the Error dialog
   => Window Two gets focused but Window One should gets focused.
   => This worked correctly before the latest commit.

[tsayao]

I reverted the last change.

After more analysis I think it's not a bug, but a side-effect of not using native window modality in favor of enabling/disabling windows.

It works like this:

On the original step 5 ("Click anywhere in Window One") the window would get the focus, but instead will send a FOCUS_DISABLED event, which will bring up the MODAL window.

The problem is that the window will get the focus, to be lost in favor of the last window (which is the MODAL one).

To prevent this, a disabled window should never be focused. But that prevents the whole FOCUS_DISABLED logic.

Note that the APPLICATION_MODAL window on the example has no owner, so I think it's ok to consider a side effect and not a bug.

[arapte]

The fix look good to me.
The other behavior is observed on mac but not on windows. The platform behavior itself looks different. Not sure if should be captured in a JBS (at least for documentation purpose)

[kevinrushforth]

Looks good now.

[openjdk]

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

8284654: Modal behavior returns to wrong stage

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

• 83a46e0: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
• f534850: 8088420: JavaFX WebView memory leak via EventListener
• d677003: 8286256: Update libxml2 to 2.9.14
• 19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
• 18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
• 81e1cc3: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
• 6c6545f: 8285217: [Android] Window's screen is not updated after native screen was disposed
• 7bb4819: 8283869: Update attribution in webkit.md file
• e24eece: 8285534: Update the 3D lighting test sample
• ee7f23d: 8283318: Videos with unusual sizes cannot be played on windows
• ... and 20 more: https://git.openjdk.java.net/jfx/compare/d1110f479567c314ecb6848700bcf4552509d7e9...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.

[tsayao]

/integrate

[openjdk]

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

• 83a46e0: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
• f534850: 8088420: JavaFX WebView memory leak via EventListener
• d677003: 8286256: Update libxml2 to 2.9.14
• 19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
• 18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
• 81e1cc3: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
• 6c6545f: 8285217: [Android] Window's screen is not updated after native screen was disposed
• 7bb4819: 8283869: Update attribution in webkit.md file
• e24eece: 8285534: Update the 3D lighting test sample
• ee7f23d: 8283318: Videos with unusual sizes cannot be played on windows
• ... and 20 more: https://git.openjdk.java.net/jfx/compare/d1110f479567c314ecb6848700bcf4552509d7e9...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tsayao Pushed as commit 864792d.

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