1554490 - Firefox is not focused on first open of a NEW Profile

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[therube]

Firefox is not focused on first open of a NEW Profile

STR:
Create a new Profile
Open FF using that Profile
(do nothing further)

Results:
FF opens
It opens with two tabs

1. a new tab, "take it with you" & (some sort of crap about) "email"
2. a background tab, "privacy notice"

At that point, FF is not focused.
Hit Alt+F4.

Expected:
FF closes.

Results:
Something may close, but it will not be FF.

Comment 1

[therube]

Easier "seen" this way, from a command prompt:

Create a new directory, C:\TESTPROFILE
Change into that new directory
C:\TESTPROFILE> start "" "C:\FIREFOX67\firefox.exe" -profile C:\TESTPROFILE

Results:

FF opens
(do nothing further at that point)

Now, type DIR <CR>

Expected: FF should say, what the heck?
Results: The DIR command is executed at the command prompt from which FF was initiated from, C:\TESTPROFILE, in this case.

---

Instead of DIR type in instead RD /S /Q <CR> :evilgrin:

Comment 2

[therube]

Not sure when this first started happening, offhand?

FF 52, opens focused to the "default browser" dialog.
FF 56, opens focused to "/firstrun/" page.

(At the least) FF 66 & FF 67 open, NOT focused to FF.

Comment 3

[Alice0775 White]

I can reproduce the issue with STR of comment$1 on Nightly69.0a1 windows10.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f017a427c3cbccf397c9bf960a7b84ae5e801cb9&tochange=067c4b7f2ed03356f5cfdf5ab6e94527053ae458

Regressed by:
067c4b7f2ed03356f5cfdf5ab6e94527053ae458 Jared Wein — Bug 1211647 - Runtime graphics testing visible during session restore. r=mchang

:jaws,
Your patch seems to cause the regression. Can you please look into this?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Moving to Graphics given the regression range in Comment 3.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Adam, jaws says he probably won't have cycles to look at this bug any time soon. Might you be able to?

Comment 6

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I suspect this is similar to

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Oops, submitted too soon.

I guess this is similar to bug 1451366 and bug 1485505, where we're falling afoul of the foregrounding heuristics.

I had thought that it was related to relaunching Firefox after creating the profile, but that doesn't happen with --profile. Maybe the additional startup delay when creating a fresh profile is involved? Bug 1451366 was very timing-sensitive. The launcher should be waiting around long enough for this to not be a problem, though.

I'll look around a little more, but I don't have much bandwidth to commit to this.

Comment 8

[:Gijs (out for now; he/him)]

Adam, just to check... this bug was traced as having regressed when we changed the graphics testing window to not appear in the taskbar (bug 1211647), which changed things such that the graphics testing window gets created with the popup window feature, which means that in widget land, as part of nsWindow::Show, we call ::ShowWindow with SW_SHOWNOACTIVATE (presumably, only for that popup graphics testing window).

The bug only happens in a new profile (where we're guaranteed to show the graphics testing window before we show the browser window). So it seems like the changes to the graphics window somehow interfere with how we're showing the "main" Firefox window.

Does that make the cause/fix more obvious to you? (I mean, it seems intuitively plausible to me that the regressing bug caused issues here, but I know too little about widget to have concrete ideas on how to fix it. If the above still isn't sufficient help to resolve this quickly, that's also fine, I just figured I'd point this out in case it helped...)

Comment 10

[Julien Wajsberg [:julienw]]

Note that this happens when a user installs Firefox and it runs for the first time. Not the best experience to have Firefox behind other windows, which is likely the case because the user downloaded the Firefox installer from another browser that should still be running.

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

There are two different issues, I think:

• Sometimes a newly opened window will appear in the background, I see this mostly with the installer, and for that there's bug 1485505.
• When Firefox starts and creates a new profile, it appears in front of other windows, but it does not have focus until it is clicked on. That's this bug, which was regressed by changing a property of the hidden graphics testing window.

Julien, did you ever see the Firefox Installer in the foreground, or did it run entirely in the background and then also launch Firefox in the background? In that case the issue is with the installer (bug 1485505), not this regression; a non-foreground program generally won't be able to launch another program that does go to the foreground. Thanks for the report either way, I keep forgetting to get back to these two issues.

Comment 12

[Julien Wajsberg [:julienw]]

Yeah, the Firefox installer was in the foreground.
Then Firefox was launched, and was behind the Chrome window that I used to download the stub in the first place. I didn't check the focus state, sorry :-(

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

We could still potentially take a patch in 72 but time is getting short. Is this something you might work on for 73 or 74?
Or, Jessie can you help find someone who can take this on?

Comment 14

[Jessie [:jbonisteel] pls NI]

Looks like this was regressed by bug 1211647

Blake, I think Jared is on your team? Do you know who can look at this regression since they are on leave? I don't think this is something the Graphics team can take on.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #11)

• When Firefox starts and creates a new profile, it appears in front of other windows, but it does not have focus until it is clicked on. That's this bug, which was regressed by changing a property of the hidden graphics testing window.

So AFAICT all this is to do with the flags we pass into ::SetWindowPos and/or ::ShowWindow. We don't want the graphics window to get a taskbar item, as the animation there is distracting/confusing - for this reason, we started using SW_SHOWNOACTIVATE. We deliberately show it off-screen so it doesn't distract the user otherwise, either.

Is there any way (ie any set of flags, or additional/different calls) we can use here to keep that behaviour but still bring Firefox to the front when the normal browser window appears?

(In reply to Jessie [:jbonisteel] plz needinfo from comment #14)

Blake, I think Jared is on your team? Do you know who can look at this regression since they are on leave? I don't think this is something the Graphics team can take on.

To be clear, Jared just landed Mason's patch when it no longer broke tests, having verified that it did what we wanted (ie not create a taskbar item for the graphics testing window). Despite that getting him assigned to the bug, I don't think the frontend folks are best-placed to fix this issue.

Comment 16

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Sorry for the slow followup, I did some investigation but got stumped.

The issue shows up when there's a new profile, because the graphics test needs to be run then. It also appears if the graphics driver has changed, which is what aklotz noticed in bug 1211647.

Yes, removing SWP_NOACTIVATE from the SetWindowPos() call here seems to fix the issue. But I don't think I can do that for all popups, and I'd rather avoid adding some extra state that has to I have to plumb all the way through. Also popups really don't want to be activated, so this might break in other ways.

The popup is interacting poorly with the CanTakeFocus() logic for toplevel windows, which is meant to prevent a normal window from stealing focus from another app that we just launched (I think, see bug 259816). We can take focus if our process has no visible window yet (so that the first window gets focus), or if the foreground window belongs to our process. But since the popup refuses to be activated, our first window is a visible window that is not in the foreground, so CanTakeFocus() calls return false.

It's actually more odd to me that we do sometimes get focus, there seem to be three modes:

1. Bad: The window is at the top of the Z order, but inactive (and not focused)
2. Bad: As in 1, but some time later gets its non-client region repainted as if it was activated, but is sent back in the Z order (and still not focused)
3. Good: As in 1, but some time later it is activated and focused

That "some time later" event that gets the window properly activated seems to also be forcing us back sometimes. I feel like this is moving us behind the foreground window, but I can't figure out why that would happen, after eliminating many suspects (DealWithPopups(), HideWindowChrome(), PlaceBehind(), "enforce local z-order rules" in OnWindowPosChanging(), etc).

A simple fix is to ignore popups in CanTakeFocus(). This seems to work, but it will require testing to make sure it doesn't break the original focus stealing prevention. I'd really like to figure out what is semi-activating the window, though, and why it is behaving so oddly. I'll try to take another look later this week, as it may suggest another fix (or another bug).

Comment 17

[Blake Winton (:bwinton) (:☕️)]

(I feel like Gijs and Adam have answered the stuff I was needinfo-ed for, so I'm clearing the flag. 🙂)

Comment 18

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

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

So AFAICT all this is to do with the flags we pass into ::SetWindowPos and/or ::ShowWindow. We don't want the graphics window to get a taskbar item, as the animation there is distracting/confusing - for this reason, we started using SW_SHOWNOACTIVATE. We deliberately show it off-screen so it doesn't distract the user otherwise, either.

A point of clarification: SW_SHOWNOACTIVATE (or SWP_NOACTIVATE) just keeps the popup from stealing focus from the normal window (which is what is so confusing in this case when there is no visible window). The WS_EX_TOOLWINDOW style used on popups is what prevents the popup from appearing in the taskbar (or alt+tab window list).

Note for testing: The bug doesn't occur when the screen is so small that the window is created maximized; this was the situation in a VM I was testing with and it stumped my early repro attempts. I think this is because we don't worry about focus stealing when showing the window maximized, and we maximize the window if we can't fit the default size of 1280x1040 with some padding.

I'm working on doing some testing now with skipping popups, I should have a patch ready later today.

Comment 19

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1554490: Don't count popups as visible windows for CanTakeFocus. r?jmathies

I did some cleanup while here: This now uses the callback's lParam
instead of a global, and does the enumeration of all windows only if
the foreground window doesn't already belong to this process (which
was proposed in bug 259816 comment 68 but didn't make it into the patch).

Comment 20

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I think that the mysterious modes 2 and 3 I reported in comment 16 were due to my use of ConEmu, I can't reproduce it with the normal Windows console or PowerShell. Not going to waste any more time with that.

We don't have the old issue with downloads since the download manager window doesn't appear anymore, but I did some testing with opening a new window when Firefox isn't focused, checking that it doesn't steal focus from another app. My patch in comment 19 seems to preserve the current behavior; Windows doesn't actually let you steal focus (anymore?), but this prevents Firefox from attempting to activate that window, which would leave the titlebar styled as if it was active.

I'm fairly satisfied that this is going to be a satisfactory fix. Ideally we'd have a new window type or flag for this, but that isn't worth the complexity for this special case, and we really don't want the graphics window to get focus anyway so this would probably be desirable anyway.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/033de5b3ae20
Don't count popups as visible windows for CanTakeFocus. r=jmathies

Comment 22

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/033de5b3ae20

Comment 23

[Ryan VanderMeulen [:RyanVM]]

I'm inclined to let this fix ride the trains unless there's a compelling reason to ship it in Fx73. LMK if you feel strongly otherwise.

Comment 24

[Daniel Bodea [:danibodea]]

I have reproduced this issue on Windows 10 with Firefox ESR v68.5.0esr (64-bit) and Firefox Release v73.0 and verified the fix with Beta v74.0b3 and Nightly v75.0a1.