809254 - New Tab (⌘T) while minimized leaves the window minimized

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jesse Ruderman]

Noticed while using Firefox Nightly 19.0a1 (2012-11-05)

1. Press ⌘M to minimize Firefox.
2. Press ⌘T to open a new tab.

Result: a new tab is opened in the minimized window, but the window remains minimized.

Expected:
* My preference is to unminimize the window to show a newly created tab.
* Another possibility is to create a new window instead.

See also: bug 542308, bug 743209 (seamonkey)

Comment 1

[Steven Michaud [:smichaud] (Retired)]

FF 18 on Windows does nothing when you Ctrl-t while it's minimized.  I suppose this is because, on Windows, FF loses focus when you minimize it.

On OS X, when you minimize an app window it keeps focus until you click elsewhere.

The simplest way to fix this bug would be to ignore Cmd-t for a window that's minimized.

Comment 2

[André Reinald]

I agree that ignoring cmt-T when the window is minimized is the simplest solution.

Though I suggest that the right solution would be to act as if that window did not exist any more, which for a multi document application on Mac OS X results in changing the menu bar accordingly : either another application window gets the focus, or no window has it (which unlike on Windows may happen in Mac OS X).

The cmd-T misbehavior is just one of many that can occur in situations where a minimized window keeps the focus. What do you think ?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> What do you think?

I'm not entirely sure what to think :-)

You make a valid point that the situation on Windows isn't exactly like the situation on the Mac.  On Windows each window has its own menu bar, but on the Mac it's each app that has its own menu bar.  So it's not possible for this bug to arise on Windows, and so emulating the behavior on Windows is just sidestepping the real problem.

In that case maybe one of the two choices from comment #0 (under "Expected") would be better.

I suggest looking at the behavior of other browsers on the Mac (like Safari, Chrome and Opera), and seeing if there's a pattern.  If there *is* a pattern (if they all behave the same way), maybe it'd be best just to imitate that.

Comment 4

[André Reinald]

Right, I checked out and the usual behavior of most Mac apps (not only browser) is one of the 2 mentioned in comment #0. Though it takes much more care to prevent weird commands from being sent down to the minimized window (like a "paste" in an active text field for example).

The behavior I was suggesting, even if sensible, would likely confuse some users.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

So maybe my suggestion from comment #1 is best, after all -- just ignore Cmd-t for a window that's minimized.

I don't think it'd be a good idea to change the menu bar (for example by forcing Firefox to give up its app focus) -- that'd be very likely to confuse users.  (I don't know if this is what you were referring to, when in comment #4 you said that "the behavior I was suggesting ... would likely confuse some users".)

I think it'd also make sense to ignore a "copy" or "paste" command in a minimized window.  And maybe you'll think of other, similar commands that should be ignored.  Then we can repurpose this bug to fix the larger problem.

It's possible that people may object to this strategy, and come up with good arguments against it.  If they do, we can change again.  But for now I think this is the way to go.

Comment 6

[André Reinald]

Giving up the app focus was not exactly what I suggested. I was more thinking of putting all the menus in the same state as if no firefox window was open (whether it means changing the menu bar or just graying out menu items is another question).

OpenOffice for instance completely changes the menu bar when no window is open (it doesn't change it though when the last window is minimized), and only has "new ... " items in the File menu at that point. But this is an unusual behavior for a Mac application.

I really think that in Firefox (or in our framework if it's used throughout other applications as well (Thunderbird ?)) we should have a "state" which indicates "focus is on a minimized window", so that menu items (or menu bar as we decide) can adjust accordingly.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

OK, you've come up with a good argument against what I suggested in comment #5.  I'd forgotten about the state of the menus (as opposed to the menu bar).

If other apps (like OpenOffice) don't change their menu state when their last window is minimized, that's probably not the way to go.

So let's just fix this bug, and do so by choosing one or the other option under "Expected" from comment #0.

We may decide at some point that we *should* do something special (and unusual) when the last window is minimized ... but let's save that for later :-)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> OpenOffice for instance completely changes the menu bar when no
> window is open

Firefox doesn't appear to do this.  Neither does Safari.

But I think OpenOffice's behavior does make some sense.  This is
another issue to consider later (aside from the issue of what menu
changes to make when the last window is minimized).

Comment 9

[André Reinald]

I understand FF user interface is based on JS, and I don't master JS. But looking through the sources and searching symbols, I found that https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#719 looks like the right place to fix the bug : is it possible to send a "unminimize" message to "aOwner" (I guess it's the window) ?

Comment 10

[André Reinald]

Attachment: Fixes the bug, the window is un-minimized when a new tab is created.

Got some help from Paul Adenot, and learned a few things in the process.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 708550 [details] [diff] [review]
Fixes the bug, the window is un-minimized when a new tab is created.

This looks fine to me, but I'm not the best reviewer for your patch (I don't know the surrounding code very well).

I'll switch the review to Däo Gottwald.  He's a more appropriate reviewer than I am.  But he can switch the review in turn if he thinks that makes sense.

I notice you haven't included your patch in an #ifdef XP_MACOSX block.  I also don't know if you've tested on other platforms (like Windows and Linux).  Since (as far as I know) you still don't have access to the tryservers, I'll do an all-platform tryserver build for you to play with.

Deciding who to ask for a review is a black art.  It's mainly a matter of experience.  If all else fails you can ask the module owner (https://wiki.mozilla.org/Modules) for a review, and assume he/she will (if need be) transfer the review to someone appropriate.  But you should also look at the "hg blame" for the surrounding code, which will give you names of people who've recently landed patches there.  That's where I found Däo's name.

Here's "hg blame" output for browser/base/content/tabbrowser.xml:
http://hg.mozilla.org/mozilla-central/annotate/cbeab7da0e3a/browser/base/content/tabbrowser.xml

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Here are the tryserver builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d107accde403/

They were supposed to show up here automatically.  I don't know why they didn't.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 708550 [details] [diff] [review]
Fixes the bug, the window is un-minimized when a new tab is created.

We already have a few places that attempt to raise the window when appropriate. For this bug I think you want to patch the openUILinkIn function in utilityOverlay.js to raise the target window in the isBlankPageURL(url) case, as the comment on why the window shouldn't be raised ("don't raise the window, since the URI we just loaded may have resulted in a new frontmost window") doesn't apply here.

Comment 14

[André Reinald]

Attachment: Fixes the bug, the window is un-minimized when a new tab is created.

Take into account https://bugzilla.mozilla.org/show_bug.cgi?id=809254#c13

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 714666 [details] [diff] [review]
Fixes the bug, the window is un-minimized when a new tab is created.

>   // content but don't raise the window, since the URI we just loaded may have
>   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
>   var fm = Components.classes["@mozilla.org/focus-manager;1"].
>              getService(Components.interfaces.nsIFocusManager);
>   if (window == fm.activeWindow)
>     w.focus();
>   w.gBrowser.selectedBrowser.focus();
> 
>-  if (!loadInBackground && isBlankPageURL(url))
>+  if (!loadInBackground && isBlankPageURL(url)) {
>     w.focusAndSelectUrlBar();
>+    if (w.windowState == w.STATE_MINIMIZED) {
>+      w.restore();
>+    }
>+  }

w.focus() a few lines above already does the same (raise the window). You can just add isBlankPageURL(url) as a condition there. Please also update the comment at the top.

Comment 16

[André Reinald]

Neither focus() nor focusAndSelectUrlBar() do un-minimize the window. We need to call restore() in case the window is minimized. And doing so in that condition block (!loadInBackground && isBlankPageURL(url)) seems like the right place. If you have a better suggestion as to where to call restore() from, please advise, but we can't skip that call.

Comment 17

[Dão Gottwald [:dao]]

focus() un-minimizes the window on Windows 7. If it doesn't on OS X, that seems like a platform-parity bug.

Comment 18

[André Reinald]

Is the semantic of focus() well defined? In which case should we change the Mac or Windows implementation of that function? From what I read here, the bug never shows on Windows because the user has no means of creating a new tab when the window is minimized.

Comment 19

[Dão Gottwald [:dao]]

(In reply to André Reinald from comment #18)
> Is the semantic of focus() well defined?

No, it's not part of any specification. It's supposed to bring windows to the front, which I think only makes sense when they're also restored from the minimized state. We already expect this when we call w.focus() in your patch's context.

> From what I read here, the
> bug never shows on Windows because the user has no means of creating a new
> tab when the window is minimized.

On Windows, you can't hit Ctrl+T to open a tab in a minimized window, but there are other ways. That's why w.focus() is already there.

Comment 20

[André Reinald]

Attachment: Fixes the bug, the window is un-minimized when a new tab is created.

Take into account comment 15

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 717050 [details] [diff] [review]
Fixes the bug, the window is un-minimized when a new tab is created.

>+  if (window == fm.activeWindow || isBlankPageURL(url))
>     w.focus();

So does this work on OS X now or do we need a bug filed to make it work?

Comment 22

[André Reinald]

It works on Mac OS X, but I wonder if it breaks something on other platforms.

I'd have preferred if focusAndSelectUrlBar() actually did what it says, which starts with focus(). But it doesn't.

If I change the implementation of focusAndSelectUrlBar() to include a call to focus(), in some cases focus() would be called twice. Unless I also change the caller code everywhere in the sources.

It's all a matter of semantics, we have to decide exactly what each function should do. The pity is that the semantics can't even be guessed from the names. That may not be an issue for experienced contributors, but it is for me now.

Comment 23

[Dão Gottwald [:dao]]

(In reply to André Reinald from comment #22)
> I'd have preferred if focusAndSelectUrlBar() actually did what it says,
> which starts with focus(). But it doesn't.

It focuses the URL bar within that window. It's not supposed raise the window.

> If I change the implementation of focusAndSelectUrlBar() to include a call
> to focus()

We don't want to raise the window in all situations, as the comment above this code explains. Which reminds me: You need to update that comment to reflect your code change.

Comment 24

[André Reinald]

Attachment: Fixes the bug, the window is un-minimized when a new tab is created.

Changed comment to reflect my changes.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 717155 [details] [diff] [review]
Fixes the bug, the window is un-minimized when a new tab is created.

Thanks!

Comment 26

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f613612cb05c

Comment 27

[Ed Morley [:emorley]]

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

Comment 28

[Tiziana Sellitto [:tiziana]]

Verified fixed with the latest mozilla central,on Mac OSX 10.8.3. The window is un-minimized when a new tab is created.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130513 Firefox/23.0
Build ID: 20130513031057