8306121: Scene not rendered initially when changing scenes after fix for JDK-8296621

[tsayao]

I replicated the fix on #1054 on Linux.

Also fixes Monocle: Stage no longer gets focus after fix for JDK-8296621

---

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-8306121: Scene not rendered initially when changing scenes after fix for JDK-8296621 (Bug - P2)

Reviewers

• Jose Pereda (@jperedadnr - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1110

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

Using diff file

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

Webrev

Link to Webrev Comment

[kevinrushforth]

I changed it to use _updateViewSize so it's compatible with other platforms.

As it turns out, I was looking at your incremental diff and (mistakenly) thought that _updateViewSize was in the current mainline code base and that you had removed it, which is why I asked about it. I now see that what happened is that the initial version of your PR had added it, and a later version had reverted that addition. And your most recent version added it back in. I don't mind either way, so if you think it's cleaner to implement _updateViewSize on Linux (even though today it's only implemented on Windows), that's OK. If you want to revert this latest addition of that method, that's OK, too.

I'm gathering some information to answer about the test changes.

Thanks.

[tsayao]

It seems Headless monocle was not focusing the window before, so this is why tests were passing before (focus was not being accounted).

I've attached a simple FocusTest on JBS that prints to the console when Stage/TextField are focused. It does not print anything before this change.

java -Dglass.platform=Monocle -Dmonocle.platform=Headless -Dprism.order=sw @build/testrun.args FocusTest.java

UPDATE:
Restoring JDK-8296621 makes Monocle focus work too. And tests passes.

UPDATE2:
It has something to do with order of events. Now the stage gets focused after shown. Somehow the testAnchorRight was getting the bounds without accounting the focus shadow size. And with the change of event order, it now gets it after. I still don't know how exactly this happens.

[tsayao]

I confirm that even restoring JDK-8296621, the tests were not getting focus.

I did this by adding to MouseEventFirerTest on setup:

        topLeft.focusedProperty().addListener((observable, oldValue, newValue) -> {
            System.out.println("topLeft focused " + newValue);
        });

        stage.focusedProperty().addListener((observable, oldValue, newValue) -> {
            System.out.println("Stage focused " + newValue);
        });

And adding testLogging.showStandardStreams = true; to build.gradle

With this PR, focus messages are shown, without the PR or restoring JDK-8296621 they don't.

Probably something to do with Monocle flow.

[tsayao]

@kevinrushforth Got it.

Testing uses StubToolkit and not QuantumToolkit which before JDK-8296621 called requestFocus() inside WindowStage which is part of QuantumToolkit. So focusing were never taken into account while testing.

[kevinrushforth]

Thanks for the explanation about why you had to change the tests.

The latest fix looks good. All my testing is green.

Running the new StartIconified manual test reveals two problems:

1. On Windows there is a brief flash where the window is visible before it is iconified -- we should file a P4 bug
2. On macOS 13 (not sure about 12) the Stage is not iconified at all -- this is tracked by JDK-8305675

[kevinrushforth]

@jperedadnr This should now be ready for you to resume your review.

[kevinrushforth]

@jperedadnr This should now be ready for you to resume your review.

When you do, can you confirm whether JDK-8304734 and JDK-8304476 are also fixed? If so, we can close them as a duplicate of JDK-8306121.

[kevinrushforth]

I filed https://bugs.openjdk.org/browse/JDK-8310029 to track the brief flash on Windows platforms.

[jperedadnr]

Looks good
Tested on macOS and Linux (with known issue on macOS), and Android, so indeed https://bugs.openjdk.org/browse/JDK-8304476 can be closed as duplicated.

I haven't tested Monocle/TestFX though.

I have minor comments only.

[jperedadnr]

This works fine on Android.

[kevinrushforth]

Thanks for confirming. I just now closed JDK-8304476 as a duplicate of JDK-8306121.

[tsayao]

@jperedadnr Did the changes requested in review.

[jperedadnr]

Looks good to me!

[kevinrushforth]

Copyright year changes are incorrect.

[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:

8306121: Scene not rendered initially when changing scenes after fix for JDK-8296621

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

• 8d13ba9: 8310024: Skip failing scene change tests on macOS
• c20f6d0: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs
• 72be85e: 8304922: [testbug] SliderTooltipNPETest fails on Linux
• 614dc55: 8306021: Add event handler management to EventTarget
• bd24fc7: 8309508: Possible memory leak in JPEG image loader
• 9913b23: 8306648: Update the JavaDocs to show the NEW section and DEPRECATED versions
• 9ad0e90: 8309470: Potential performance improvements in VirtualFlow
• 6a159e9: 8194704: Text/TextFlow hitTest() javadoc
• 883c4e0: 8309001: Allow building JavaFX on Linux/riscv64
• 17ed2e7: 8307538: Memory leak in TreeTableView when calling refresh
• ... and 1 more: https://git.openjdk.org/jfx/compare/10f41b7d1f2f53ebe2bfdb61de495bbb7290d32d...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 7eb9a1c.
Since your change was applied there have been 11 commits pushed to the master branch:

• 8d13ba9: 8310024: Skip failing scene change tests on macOS
• c20f6d0: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs
• 72be85e: 8304922: [testbug] SliderTooltipNPETest fails on Linux
• 614dc55: 8306021: Add event handler management to EventTarget
• bd24fc7: 8309508: Possible memory leak in JPEG image loader
• 9913b23: 8306648: Update the JavaDocs to show the NEW section and DEPRECATED versions
• 9ad0e90: 8309470: Potential performance improvements in VirtualFlow
• 6a159e9: 8194704: Text/TextFlow hitTest() javadoc
• 883c4e0: 8309001: Allow building JavaFX on Linux/riscv64
• 17ed2e7: 8307538: Memory leak in TreeTableView when calling refresh
• ... and 1 more: https://git.openjdk.org/jfx/compare/10f41b7d1f2f53ebe2bfdb61de495bbb7290d32d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tsayao Pushed as commit 7eb9a1c.

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

[dsubelman]

Thanks for fixing this issue. Does this fix also solve the Monocle/TestFX issue?

[tsayao]

It probably does fix It. But we found out that focus was not working with StubToolkit because the focus call was previously called only on QuantumToolkit. I don't know about TestFx but since the below bug exists (about focus) it probably doesn't use StubToolkit. TestFX/TestFX#749 Em sex., 16 de jun. de 2023 12:53, Daniel Subelman ***@***.***> escreveu:

…

Thanks for fixing this issue. Does this fix also solve the Monocle/TestFX issue? — Reply to this email directly, view it on GitHub <#1110 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHKIFHUPZAF3FOCBDF5TAUDXLR6OHANCNFSM6AAAAAAXH2KJHY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[kevinrushforth]

I think JDK-8304734 can be closed as a duplicate of this bug, but it would be helpful to confirm that it resolves the problem reported in TestFX/TestFX#749.