Re: [jfx17] RFR: 8240640: [macos] Wrong focus behaviour with multiple Alerts [v2]
On Fri, 23 Jul 2021 16:35:58 GMT, Kevin Rushforth wrote:
>> Pankaj Bansal has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed review comments 1) Increase delay 2) Fix Formatting 3) Make test
>> stable on Windows according to suggestions
>
> tests/system/src/test/java/test/robot/javafx/stage/WrongStageFocusWithApplicationModalityTest.java
> line 95:
>
>> 93: Util.runAndWait(() -> {
>> 94: robot.mouseMove((int) (alert.getX()+alert.getWidth()/2),
>> 95: (int) (alert.getY()+alert.getHeight()/2));
>
> Minor: we usually put spaces around binary operators.
Fixed
-
PR: https://git.openjdk.java.net/jfx/pull/581
Re: [jfx17] RFR: 8240640: [macos] Wrong focus behaviour with multiple Alerts [v2]
On Fri, 23 Jul 2021 00:16:33 GMT, Kevin Rushforth wrote: > The fix (reverting the earlier fix for JDK-8227366) looks fine to me. The > test runs fine on Mac (and mostly fine on Linux, but I need to test a couple > more things). > > I also see the failure on Windows frequently depending on whether the Window > initially gets focus. One thing you might try is to call `stage.toFront()` > and `stage.requestFocus()` before showing the Alerts, although I'm not sure > that will help. For other tests like this we do set the primary stage to be > always on top, so that's probably the easiest. Failing that you could mark > the test as unstable on Windows. > > Btw, on my slow Linux VM, it occasionally fails because the window takes a > bit too long to come up. I'll do some more testing once you fix the Windows > issue. > > I left a couple formatting comments on the test inline. I tried the stage.toFront or stage.requestFocus, they did not work. So I added the stage.setAlwaysOnTop(true). Along with that, I also added functionality to do mouse click at centre of alert to make sure keyboard focus is present on the alert. I have verified that the updated test fails before the fix and passes with the fix. I have tried multiple iterations of the test on all platforms (Mac 10.15, Ubuntu 20.04 and Windows 10) and test looks stable. - PR: https://git.openjdk.java.net/jfx/pull/581
Re: [jfx17] RFR: 8240640: [macos] Wrong focus behaviour with multiple Alerts [v2]
On Fri, 23 Jul 2021 15:24:35 GMT, Pankaj Bansal wrote:
>> The bug is a regression as a result of fix done for JDK-8227366 and is
>> reproducible on Linux and Mac. This fix is being reverted in this change and
>> a new bug (JDK-8271054) has been created to redo the JDK-8227366
>>
>> An automated testcase is being added to make sure similar regression is not
>> introduced when working on the redo bug. The automated testcase fails
>> without the current change and passes after the change.
>>
>> The testcase works fine on Mac and Linux, but I see some issues in windows.
>> The stage is minimised after calling stage.show without calling
>> stage.setAlwaysOnTop(true). I see that there are other tests like
>> DualWindowsTest.java which are failing for same reason. Also, if I remove
>> the stage.setAlwaysOnTop(true) from tests like
>> FocusParentWindowOnChildCloseTest.java, the stage remains minimised there
>> as well. This seems wrong as calling stage.setAlwaysOnTop(true) should not
>> be required to bring stage window in focus on running the test. This may be
>> an issue on my Window machine or something similar. I hope reviewers can
>> shed some light on this.
>
> Pankaj Bansal has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed review comments 1) Increase delay 2) Fix Formatting 3) Make test
> stable on Windows according to suggestions
The test changes look good (one minor formatting comment), and it now runs
reliably for me on all three platforms.
tests/system/src/test/java/test/robot/javafx/stage/WrongStageFocusWithApplicationModalityTest.java
line 95:
> 93: Util.runAndWait(() -> {
> 94: robot.mouseMove((int) (alert.getX()+alert.getWidth()/2),
> 95: (int) (alert.getY()+alert.getHeight()/2));
Minor: we usually put spaces around binary operators.
-
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/581
Re: [jfx17] RFR: 8240640: [macos] Wrong focus behaviour with multiple Alerts [v2]
> The bug is a regression as a result of fix done for JDK-8227366 and is > reproducible on Linux and Mac. This fix is being reverted in this change and > a new bug (JDK-8271054) has been created to redo the JDK-8227366 > > An automated testcase is being added to make sure similar regression is not > introduced when working on the redo bug. The automated testcase fails without > the current change and passes after the change. > > The testcase works fine on Mac and Linux, but I see some issues in windows. > The stage is minimised after calling stage.show without calling > stage.setAlwaysOnTop(true). I see that there are other tests like > DualWindowsTest.java which are failing for same reason. Also, if I remove the > stage.setAlwaysOnTop(true) from tests like > FocusParentWindowOnChildCloseTest.java, the stage remains minimised there as > well. This seems wrong as calling stage.setAlwaysOnTop(true) should not be > required to bring stage window in focus on running the test. This may be an > issue on my Window machine or something similar. I hope reviewers can shed > some light on this. Pankaj Bansal has updated the pull request incrementally with one additional commit since the last revision: Fixed review comments 1) Increase delay 2) Fix Formatting 3) Make test stable on Windows according to suggestions - Changes: - all: https://git.openjdk.java.net/jfx/pull/581/files - new: https://git.openjdk.java.net/jfx/pull/581/files/6d727cd4..623c3930 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=581=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=581=00-01 Stats: 20 lines in 1 file changed: 16 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/581.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/581/head:pull/581 PR: https://git.openjdk.java.net/jfx/pull/581
