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

RESOLVED FIXED in Firefox 61

Status

RESOLVED FIXED
2 years ago
23 days ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks: 1 bug, {regression})

Version 3
mozilla62
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox60 wontfix, firefox61 fixed, firefox62 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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?
Flags: needinfo?(enndeakin)
I think this conflicts with the change in 887718. Does it work with that patch removed?
Flags: needinfo?(enndeakin)
(Assignee)

Comment 2

2 months ago
(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.
Blocks: 887718, 1398111
Flags: needinfo?(enndeakin)
Keywords: regression
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 months ago
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.
Flags: needinfo?(enndeakin)
(Assignee)

Updated

2 months ago
Attachment #8984131 - Flags: review?(enndeakin)
(Assignee)

Comment 5

2 months ago
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.
(Assignee)

Updated

2 months ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #8984131 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 6

2 months ago
Hm, the r+ is not available in mozreview so I cannot land myself. Can someone please land the patch? Thanks.
status-firefox61: --- → affected
status-firefox62: --- → affected
status-firefox-esr60: --- → fix-optional
Keywords: checkin-needed
:Neil: could you please grant review in mozreview tool too?
Flags: needinfo?(gbrown)
Flags: needinfo?(enndeakin)

Comment 8

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

https://reviewboard.mozilla.org/r/249954/#review256462
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 9

2 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f16a2ff45ab
Backed out changeset d6ca24ba3673 for regression caused by bug 887718. r=enndeakin+6102

Comment 10

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f16a2ff45ab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Flags: needinfo?(enndeakin)
(Assignee)

Comment 11

2 months ago
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
Flags: needinfo?(gbrown)
Attachment #8984131 - Flags: approval-mozilla-beta?
status-firefox60: --- → wontfix
status-firefox-esr52: --- → wontfix
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.
Attachment #8984131 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

2 months ago
Whiteboard: [checkin-needed-beta]

Comment 13

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9c3ebbc12a02
status-firefox61: affected → fixed
Whiteboard: [checkin-needed-beta]
status-firefox-esr60: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.