This was initially supposed to be handled in Bug 1533058, but for now we will keep using BrowserOpenTab to open tabs on Firefox desktop in marionette/browser.sys.mjs openTab.

We should consistently use TabManager.addTab to create tabs in the remote codebase, but doing so seems to make some tests intermittently fail.

We should also use the occasion to review which API should be used behing the scenes to open the tab. Right now TabManager.addTab uses tabBrowser.addTab, but maybe we should rely on window.BrowserOpenTab

By using BrowserOpenTab() to open a new tab in Firefox there is also the browser-open-newtab-start notification that gets send out for performance tracking. If we stop using this method and instead use gBrowser.addTab() we would loose that (or if needed would have to send it out ourselves?).

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

If we are going with gBrowser.addTab() we might have to wait for the Ubuntu 22.04 upgrade of test machines (bug 1725245) because using this API causes test failures with focus on Linux machines in CI only.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

By using BrowserOpenTab() to open a new tab in Firefox there is also the browser-open-newtab-start notification that gets send out for performance tracking. If we stop using this method and instead use gBrowser.addTab() we would loose that (or if needed would have to send it out ourselves?).

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

Yeah, I don't think the notification is relevant.

If we are going with gBrowser.addTab() we might have to wait for the Ubuntu 22.04 upgrade of test machines (bug 1725245) because using this API causes test failures with focus on Linux machines in CI only.

Do we know why? This smells like a race condition of some kind in marionette. The whole thing doesn't make much sense because BrowserOpenTab still ends up calling addTab at some point down the line. So presumably the only difference between the case where we lose the race and where we win the race is a parameter of some kind? I guess bug 1533058 comment 38 asked the same question.

From looking at that discussion and the issues that only reproduce on infra, I can't quite figure out whether there was analysis done in terms of what the difference for the two calls to addTab ends up being? That seems like the most straightforward way to resolve the potential difference so you end up with an identical call to addTab... you can just use local logging - you don't need to reproduce any failures on infra.

I guess if this switches to gBrowser.addTab we can revert the change in bug 1533058 to allow overriding the url for BrowserOpenTab...

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

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

Yeah, I don't think the notification is relevant.

Thanks. That gives me confidence.

Do we know why? This smells like a race condition of some kind in marionette. The whole thing doesn't make much sense because BrowserOpenTab still ends up calling addTab at some point down the line. So presumably the only difference between the case where we lose the race and where we win the race is a parameter of some kind? I guess bug 1533058 comment 38 asked the same question.

Sadly not given that any additional logging that I was using on try was not helpful yet to figure out the issue. Also interactive tasks were broken since the infra moved to GCP. With bug 1786307 fixed yesterday I'm finally able to use interactive tasks again. I might have the time to start looking at this bug by next week. It's not a priority for now so it might take a bit.

From looking at that discussion and the issues that only reproduce on infra, I can't quite figure out whether there was analysis done in terms of what the difference for the two calls to addTab ends up being? That seems like the most straightforward way to resolve the potential difference so you end up with an identical call to addTab... you can just use local logging - you don't need to reproduce any failures on infra.

I would like to see the real effect of behavior in CI when I make changes to the code. That would make it faster in discovering at which layer on top of gBrowser.addTab() the broken behavior gets introduced. Once found me can use several MOZ_LOG features (like focus, focus manager, widget) to check the differences of focus handling.

I guess if this switches to gBrowser.addTab we can revert the change in bug 1533058 to allow overriding the url for BrowserOpenTab...

Yes, absolutely!

Maybe my patch on bug 1798655 might have helped here. As we noticed BrowerOpenTab() doesn't actually focus the tab's content but sets the focus to the chrome window. If code in content then tries to focus an element it will take longer and you cannot assume that it's focused when the call to focus() returns. As such we explicitly set the focus to the linkedBrowser now when switching between tabs.

I pushed a try build to check if it works now with the focus fix included:
https://treeherder.mozilla.org/jobs?repo=try&revision=2e10b64d972b1d927281572467d15fe248457620

As the try build shows the issue with scrolling an element into view is still remaining on Linux shippable builds. As such we will pick-up this bug again when there is a bit more time or maybe when we got new Linux workers with a more up-to-date Ubuntu release.