2007691 - After minimizing and unminimizing Firefox window, new container tabs open in previously active window instead of currently focused window

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[andrew]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:146.0) Gecko/20100101 Firefox/146.0

Steps to reproduce:

1. Activate multi-account containers
2. Open two windows
3. Minimize the second window, then restore it
4. In the restored second window, click the multi-account containers toolbar icon and then "Open New Tab In" and then choose a container
5. Notice how the container tab opens incorrectly in the previously active window, not the focused window that you clicked the toolbar icon in.

Actual results:

New container tab opens in wrong window - the previously active window - instead of the focused window

Expected results:

New container tab should have opened in the focused window

I have been able to consistently reproduce this issue on macOS Tahoe and Sequoia. I have NOT been able to reproduce this issue on Windows. This suggests it is a bug with the macOS window management stuff. If the bug is on the Apple side, Firefox should workaround this very annoying issue.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Cocoa' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Stephen A Pohl [:spohl]]

I believe this belongs in the WebExtensions component. Please reassign as you see fit.

Comment 3

[Rob Wu [:robwu]]

Although extensions are affected, the root cause is not in extension code. We chose the window to open the tab in here, which ends up calling BrowserWindowTracker.getTopWindow(...).

The BrowserWindowTracker.getTopWindow() method enumerates the _trackedWindows list to look up the most recently focused windows. When a window is raised, its "activate" window is dispatched, and supposedly the window should be in the front of the list (link to implementation). But if window.windowState is still at window.STATE_MINIMIZED, then the window won't move to the front of the list. This is the case, as we see below.

If I reduce the STR to the following, I can still reproduce:

1. Visit about:config, and set devtools.chrome.enabled to true.
2. Open a new window.
3. Open the Browser Console and run the following code:

w=window;
w.addEventListener("activate",e=>console.log(e.type,e.target.windowState))
w.minimize();
setTimeout(()=>w.restore(),2000)
setTimeout(()=>console.log(BrowserWindowTracker.getTopWindow().gBrowser.selectedBrowser.currentURI.spec), 3000);

4. When the window (from step 2) minimizes, focus the window (from step 1).
5. Wait 3 seconds (until the tab is restored, and another second).
6. Look at the console.

Expected (e.g. on Linux):

• activate 3 (where 3 means STATE_NORMAL)
• about:home (from step 2)

Actual (on macOS):

• activate 2 (where 2 means STATE_MINIMIZED)
• about:config (from step 1).

I can think of a few approaches to fix this issue:

• Somehow ensure that window.windowState is STATE_NORMAL when activate is dispatched.
• Do not just listen for activate, but also (or only?) listen for sizemodechange event in BrowserWindowTracker.sys.mjs.

I think that it would be nice if windowState has the expected value when activate is fired. Whether by setting windowState earlier or firing activate a bit later.

Comment 4

[Rob Wu [:robwu]]

The BrowserWindowTracker.getTopWindow() method has many callers. If it is unable to reliably track the top window, then it would kind of defeat the purpose of that method.

Is it possible to fix this at the widget level?

And if not, should we consider a workaround (e.g. also monitoring the sizemodechange event in BrowserWindowTracker)?

Comment 5

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #4)

The BrowserWindowTracker.getTopWindow() method has many callers. If it is unable to reliably track the top window, then it would kind of defeat the purpose of that method.

Is it possible to fix this at the widget level?

Stephen, I think Rob was hoping for an answer to whether we could fix the dispatch of the event on macOS so it happens after the windowState is updated? :-)

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Rob Wu [:robwu] from comment #4)

The BrowserWindowTracker.getTopWindow() method has many callers. If it is unable to reliably track the top window, then it would kind of defeat the purpose of that method.

Is it possible to fix this at the widget level?

Stephen, I think Rob was hoping for an answer to whether we could fix the dispatch of the event on macOS so it happens after the windowState is updated? :-)

Bah, needinfo for the right person now, sorry!

Comment 7

[Stephen A Pohl [:spohl]]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Rob Wu [:robwu] from comment #4)

The BrowserWindowTracker.getTopWindow() method has many callers. If it is unable to reliably track the top window, then it would kind of defeat the purpose of that method.

Is it possible to fix this at the widget level?

Stephen, I think Rob was hoping for an answer to whether we could fix the dispatch of the event on macOS so it happens after the windowState is updated? :-)

Oops, I didn't mean to clear my need-info when I set an initial severity. I'm actively investigating.

Comment 8

[andrew]

Any updates to share? I am still hitting this bug constantly in daily use...

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to andrew from comment #8)

Any updates to share? I am still hitting this bug constantly in daily use...

No updates unfortunately other than the fact that this bug is still on my to-do list and I'm trying to get to this asap.

Comment 10

[Stephen A Pohl [:spohl]]

I had some time to look into this today/yesterday. Would you be able to run the following test build? You will have to follow these steps:

1. Download https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ZE8TqPwCQXeaxOAtnOIdwQ/runs/0/artifacts/public/build/target.tar.gz
2. Extract "Firefox Nightly.app" to a known location, such as ~/Downloads
3. Run the following command in Terminal and adjust the path based on where you've extracted "Firefox Nightly.app" to: xattr -r -d com.apple.quarantine ~/Downloads/Firefox\ Nightly.app
4. Run the test build and try to reproduce the issue.

Is the reported issue fixed in this test build?

Comment 11

[:Gijs (he/him)]

FWIW, as a workaround / alternative, you can right click (ctrl-click) or long-press-click the regular new tab ([+]) button and it will have a menu to open a new container tab in a specific container. I've checked the same steps from comment 0 using that instead of step 4, and it works correctly (because it's just opening a tab in the same window the button is in). You can also tick "select a container for each new tab" in the Firefox settings to make choosing a container be required whenever you click the new tab button (which may or may not be a good tradeoff for your use).

(It would still be really valuable to get this bug fixed so it'd still be useful to get an answer to Stephen's question in comment 10!)

Comment 12

[andrew]

Just tried that test build and was unable to reproduce the bug. Nice!

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Bug 2007691: Filter minimized windows from BrowserWindowTracker.getTopWindow and orderedWindows at read time. r=gijs!

Comment 14

[Stephen A Pohl [:spohl]]

(In reply to Rob Wu [:robwu] from comment #3)

[...]
I can think of a few approaches to fix this issue:

• Somehow ensure that window.windowState is STATE_NORMAL when activate is dispatched.
• Do not just listen for activate, but also (or only?) listen for sizemodechange event in BrowserWindowTracker.sys.mjs.
  [...]

Thank you, Rob, for this analysis. It definitely made it easier to write this patch. I chose your second suggestion as it seemed less likely to introduce unwanted behavior changes and/or regressions.

Comment 15

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #3)

Although extensions are affected, the root cause is not in extension code. We chose the window to open the tab in here,

Why doesn't the extension (or our extension framework) pass the correct window ID so we open the tab in the same window in which the button was clicked?

It is generally an antipattern for UI code that responds to a click somewhere to be using getTopWindow() to find out where the click happened - it should be passing along context to make that determination. Even without bugs win the window tracker (and from reviewing the patch here, there's substantial history in bug 1529577, bug 1509847, bug 1489552, etc.), as soon as any asynchronicity is introduced the focused/active window can change between when an operation starts and finishes, and so relying on it is a footgun.

Comment 16

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #15)

(In reply to Rob Wu [:robwu] from comment #3)

Although extensions are affected, the root cause is not in extension code. We chose the window to open the tab in here,

Why doesn't the extension (or our extension framework) pass the correct window ID so we open the tab in the same window in which the button was clicked?

In various extension framework's UI events we already pass the relevant windowId (and tab) to the extension event, but in this case the trigger is a regular DOM event that does not have that context.

In this specific case the click happens in a visible extension popup anchored to a window, so we could indeed try to default to the popup's window when a windowId is not specified. That doesn't fix the issue in the general case, nor situations where extensions use browser.tabs.query({ lastFocusedWindow: true, active: true }) to look up the current tab/window (e.g. when called from a background script that is not tied to a particular window), a pattern that is very common because we currently don't have an extension API that returns the popup panel's window.

So while we could theoretically try to provide even more context to extensions, it would not provide a general solution. Fixing window focus tracking would be my preferred solution here.

Comment 17

[Stephen A Pohl [:spohl]]

Setting n-i regarding my responses in phabricator. To summarize: I wonder if we should move this bug to a different component as it no longer seems like a widget bug to me.

Comment 18

[:Gijs (he/him)]

(In reply to Stephen A Pohl [:spohl] from comment #17)

Setting n-i regarding my responses in phabricator. To summarize: I wonder if we should move this bug to a different component as it no longer seems like a widget bug to me.

Sorry for the super slow response here. I do think widget has a role to play here. Slightly more context in https://phabricator.services.mozilla.com/D293563#10463392

Comment 19

[Stephen A Pohl [:spohl]]

Submitted updated patch for feedback/review. Clearing n-i.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c171e86b51bd
https://hg.mozilla.org/integration/autoland/rev/d83e3f3138cd
Filter minimized windows from BrowserWindowTracker.getTopWindow and orderedWindows at read time. r=Gijs

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/d83e3f3138cd