1334981 - If focusmanager.testmode is true, RecentWindow.getMostRecentBrowserWindow() always returns the last opened window

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I have seen this behavior while working on a Marionette test for focus handling when switching between windows (bug 1124604). So what's happening?

For Marionette tests the above preference is always turned on. Now I want to test a scenario when opening a window in the background. This is happening with window.open() and a blur() on the new window, or focus() on the former window. With the preference set you won't actually see a focus change, and that's expected.

But when you now want to retrieve the most recent browser window, I always get the newly opened window which is in the background. This happens even when I manually bring the former window into front. 

Neil, I don't think that this behavior is correct, and that getMostRecentBrowserWindow() should always return the chrome window which is indeed the top-most.

What do you think?

Comment 1

[Neil Deakin]

I think this conflicts with the change in 887718. Does it work with that patch removed?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Neil Deakin from comment #1)
> I think this conflicts with the change in 887718. Does it work with that
> patch removed?

Yes, it does! Another regression as caused by that patch actually is that it is not possible right now to switch between windows via the "Window" menu. The current window never changes, even selecting a different one from the list. I assume that this is not correct, and that we have to revert the change from bug 887718.

Sorry, that it has been taken that long to respond.

I will push a try build with the change backed out. Maybe nothing major breaks and we can easily revert.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1334981 - Backed out changeset d6ca24ba3673 for regression caused by bug 887718.

Review commit: https://reviewboard.mozilla.org/r/249954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249954/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The try build so far looks fine. For some failures I retriggered the jobs and I will wait for those, but nothing should block a review here.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The try build was nonsense given that I pushed an artifact build :( Just figured out when I tried to test locally and switching windows still doesn't work with a downloaded build. I will have to trigger again, and I assume a subset of tests would be enough.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hm, the r+ is not available in mozreview so I cannot land myself. Can someone please land the patch? Thanks.

Comment 7

[Noemi Erli[:noemi_erli]]

:Neil: could you please grant review in mozreview tool too?

Comment 8

[Neil Deakin]

Comment on attachment 8984131 [details]
Bug 1334981 - Backed out changeset d6ca24ba3673 for regression caused by bug 887718.

https://reviewboard.mozilla.org/r/249954/#review256462

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f16a2ff45ab
Backed out changeset d6ca24ba3673 for regression caused by bug 887718. r=enndeakin+6102

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1f16a2ff45ab

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8984131 [details]
Bug 1334981 - Backed out changeset d6ca24ba3673 for regression caused by bug 887718.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 887718 which landed for Firefox 25
[User impact if declined]: Users won't be able to switch between different windows. It only happens when the preference "focusmanager.testmode" is set to "true", which is not the default. Also this is usually only used in automation.
[Is this code covered by automated tests?]: Not yet, but bug 1398111 will update a test.
[Has the fix been verified in Nightly?]: Yes, manually
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Visibility of the window when switching is set everytime now, and not only outside of the test mode. 
[String changes made/needed]: No

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984131 [details]
Bug 1334981 - Backed out changeset d6ca24ba3673 for regression caused by bug 887718.

Fix for code behind a non-default pref which helps testing. Approved for 61.0b13.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c3ebbc12a02