674370 - [10.7] Support animation when opening windows in Lion

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

When opening the application on OS X Lion, we should use the same animation as the other applications. (Zoom in)

Comment 1

[Colin Barrett [:cbarrett]]

Actually the animation happens when opening any window.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Minimal WIP v0.1

Here's a minimal patch that turns the zoom animation on for new top level windows.

What is the guideline for the kinds of windows that should show the animation and the kinds that shouldn't?  My guess is that it would be for "document"-like windows (those that you can have multiple instances of).  The windows that do show the zoom animation with this patch that I think shouldn't are:

* Page Setup
* Print       (should Page Setup and Print be sheets anyway?)
* Library
* Downloads
* Web Developer > Style Editor
* Error Console
* Page Info
* Preferences

We'll also need to ensure the animations don't show when session restore is in progress, where it would be be an unnecessary distraction IMO.

There might be some yes/no ok/cancel type of dialog windows where we'd want to use NSWindowAnimationBehaviorAlertPanel (which I think does more of a bounce when zooming in), too.

Comment 3

[Cameron McCormack (:heycam)]

So keying whether to show the animation off mWindowType == eWindowType_toplevel isn't exactly right, because for example the Downloads window is eWindowType_toplevel but we don't want the animation there.

Advice on how to proceed?  Stick a new attribute on XUL <window> elements that should show the animation?

Comment 4

[Cameron McCormack (:heycam)]

Josh, do you have an opinion on my comment 3?

Comment 5

[Cameron McCormack (:heycam)]

I think that by the time I need to pass the nsWidgetInitData into the widget creation function, I won't have the XUL document with the <window> element around yet, because it won't have loaded.  Is that correct?  If so, then maybe I can mint a new chrome flag bit and set it if the right flag string is set in the window.open() call, like window.open("whatever.xul", ..., "chrome,utility").  That seems to have the disadvantage of requiring at every window.open call site, so it would be nice if I could put something on the XUL <window> element if that is possible.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: WIP v0.2

This does the comment 5 approach of adding a "utility" feature flag to open(), which is used at the call sites for opening the windows mentioned in comment 2 (except for the Page Setup and Print windows).  It might be better though to reverse the flag, and use one for the windows we *do* want to have the zooming animation for.  (So maybe "nonutility" or "document".)  Otherwise, extensions that open top level windows will probably get zooming when they don't want it (since they'll leave out "utility").

I think the common case (although I still haven't worked out what the exact rule should be, looking at other applications) is not wanting the zooming, and specifically it should just be these windows:

* Browser windows
* Open
* Save Page As

Comment 7

[Cameron McCormack (:heycam)]

Oh and the Scratchpad window, since you can have multiple of those open, and they look fairly document like.

Comment 8

[Cameron McCormack (:heycam)]

I ended up inverting the sense of the chrome flag, and I went with the name "document".  This patch makes new browser windows and Scratch pad windows zoom on opening.  I changed all browser window opening code that I could find to use the "document" flag, including tests.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v1)

Comment 10

[Cameron McCormack (:heycam)]

Try run in progress https://tbpl.mozilla.org/?tree=Try&noignore=1&rev=96fc70b972d1.  So far no more orange on the Lion builders than there is normally.

Comment 11

[Josh Aas]

Comment on attachment 584920 [details] [diff] [review]
Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v1)

Probably better to have Markus Stange review this.

Comment 12

[Markus Stange [:mstange]]

Hey Cameron, sorry for the delay here.

I'm not fond of adding the document flag to all these open() callers. I think it would be better if the decision to animate were on the side of the window itself, not on the side of its opener. A XUL attribute (maybe type="document") on the <window> element of the affected windows might be better. Such an attribute would be implemented like the toggletoolbar attribute.

Did you take the open() flag approach because of the session restore scenario? Does your patch prevent restored windows from animating?
If so, maybe we do need an open() flag to override the animation. But then the default should be "look at the window's attribute", so that the session restore call site is the only one that needs changing.

Comment 13

[Markus Stange [:mstange]]

(In reply to Cameron McCormack (:heycam) from comment #5)
> I think that by the time I need to pass the nsWidgetInitData into the widget
> creation function, I won't have the XUL document with the <window> element
> around yet, because it won't have loaded.  Is that correct?

Yes. XUL attributes that affect widget opening don't pass their information through nsWidgetInitData; instead, nsXULWindow::SyncAttributesToWidget() calls separate methods on the widget after it has been initialized (but before widget->Show() has been called).

Comment 14

[Cameron McCormack (:heycam)]

Thanks, using SyncAttributesToWidget works well here.

How about for inhibiting the animation when doing a session restore?  Would you be OK with using a chrome flag for that (that only nsSessionStore.js would pass in to openWindow)?

Comment 15

[Markus Stange [:mstange]]

Sure.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v2)

This patch uses a chrome flag to force animations off when creating a new
top-level document window.  The result of using this flag from the session
restore code is that all windows created during session restore except for
the very first one will have the animation disabled.  It would be hard to
get that first window unanimated too, since it is already created before
session restore does its thing.  I think it looks OK with that one animated
anyway, though, as long as there isn't a succession of windows all animating
in at startup.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 594590 [details] [diff] [review]
Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v2)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>diff --git a/browser/base/content/test/browser_bug422590.js b/browser/base/content/test/browser_bug422590.js
>diff --git a/browser/components/places/src/PlacesUIUtils.jsm b/browser/components/places/src/PlacesUIUtils.jsm
>diff --git a/browser/components/sessionstore/test/browser_580512.js b/browser/components/sessionstore/test/browser_580512.js

Since the changes in these files (replacing "chrome://browser/content/" with getBrowserURL()) don't really have anything to with window animation, they shouldn't be part of this patch. If you think they're worthwhile feel free to submit as a separate patch in a different bug.

>diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm
>+      if (mWindowType == eWindowType_toplevel && nsToolkit::OnLionOrLater() &&

Instead of nsToolkit::OnLionOrLater() I think it would be better to check for [mWindow respondsToSelector:@selector(setAnimationBehavior:)].

>+void nsCocoaWindow::SetTopLevelWindowType(nsIWidget::TopLevelWindowType aType)
>+{
>+  switch (aType) {
>+    case nsIWidget::eGenericWindow:
>+      mIsTopLevelDocument = false;
>+      break;
>+    case nsIWidget::eDocumentWindow:
>+      mIsTopLevelDocument = true;
>+      break;
>+    default:
>+      NS_NOTREACHED("unexpected TopLevelWindowType value");
>+  }
>+}

I'd vote for mIsTopLevelDocument = (aType == nsIWidget::eDocumentWindow) instead of the switch, but you can keep it this way if you prefer.

You'll also need to get a review for the browser XUL changes (e.g. from Dão or Gavin) and maybe a superreview for the nsIWebBrowserChrome.idl change, for example from bz.

Still, looks good, thanks for doing this!

Comment 18

[Cameron McCormack (:heycam)]

(In reply to Markus Stange from comment #17)
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> >diff --git a/browser/base/content/test/browser_bug422590.js b/browser/base/content/test/browser_bug422590.js
> >diff --git a/browser/components/places/src/PlacesUIUtils.jsm b/browser/components/places/src/PlacesUIUtils.jsm
> >diff --git a/browser/components/sessionstore/test/browser_580512.js b/browser/components/sessionstore/test/browser_580512.js
> 
> Since the changes in these files (replacing "chrome://browser/content/" with
> getBrowserURL()) don't really have anything to with window animation, they
> shouldn't be part of this patch. If you think they're worthwhile feel free
> to submit as a separate patch in a different bug.

OK I'll split them out.

> >diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm
> >+      if (mWindowType == eWindowType_toplevel && nsToolkit::OnLionOrLater() &&
> 
> Instead of nsToolkit::OnLionOrLater() I think it would be better to check
> for [mWindow respondsToSelector:@selector(setAnimationBehavior:)].

OK.

> >+void nsCocoaWindow::SetTopLevelWindowType(nsIWidget::TopLevelWindowType aType)
> >+{
> >+  switch (aType) {
> >+    case nsIWidget::eGenericWindow:
> >+      mIsTopLevelDocument = false;
> >+      break;
> >+    case nsIWidget::eDocumentWindow:
> >+      mIsTopLevelDocument = true;
> >+      break;
> >+    default:
> >+      NS_NOTREACHED("unexpected TopLevelWindowType value");
> >+  }
> >+}
> 
> I'd vote for mIsTopLevelDocument = (aType == nsIWidget::eDocumentWindow)
> instead of the switch, but you can keep it this way if you prefer.

At some point we might want to introduce other values for toplevelwindowtype="" (and the enumeration here) to elicit the other kinds of animation types that Lion supports.  That's the only reason I have a switch here.  I think I'll leave it for now (but in general I agree with you that a simple conditional is better than switching to assign a boolean).

> You'll also need to get a review for the browser XUL changes (e.g. from Dão
> or Gavin) and maybe a superreview for the nsIWebBrowserChrome.idl change,
> for example from bz.

OK.

> Still, looks good, thanks for doing this!

Thanks for the help and quick review!

Comment 19

[Cameron McCormack (:heycam)]

Boris, can you superreview my chrome flag changes here?

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 594590 [details] [diff] [review]
Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v2)

>+        toplevelwindowtype="document"

Can this be renamed to something like macenableanimation="true"?

--- a/browser/components/sessionstore/src/nsSessionStore.js
>+++ b/browser/components/sessionstore/src/nsSessionStore.js

>-    let features = "chrome,dialog=no,all";
>+    let features = "chrome,dialog=no,unanimated,all";

And macsuppressanimation?

Naming these attributes as if they made sense cross-platform doesn't seem useful and might lead to unintended interpretations of them.

Comment 21

[Cameron McCormack (:heycam)]

I can make them sound more Mac-specific.  I'd rather use something like macanimtionattype="document", though, in case we add "utility" and "alert" in the future.  If I do this, should I #ifdef them out, or just leave them for all platforms and make them have no effect (as the patch does currently)?

Comment 22

[Cameron McCormack (:heycam)]

(Modulo extreme typos of course.)

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v3)

Go with macanimationtype="" as the XUL attribute and "macsuppressanimation" as the openWindow flag.  To be consistent, I renamed the chrome flag constant.  I also reworked the nsCocoaWindow part of it to store the animation type rather than a boolean, since we'll probably need to do that when we support "utility" and "alert" anyway.  (mstange, do you mind re-reviewing in light of those changes?  Thanks!)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594879 [details] [diff] [review]
Make browser windows (and other document-like windows) zoom on creation on OS X 10.7. (v3)

Chrome flag change is fine, if you document what the flag means.

Comment 25

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8b935ab596

Comment 26

[Marco Bonardo [:mak]]

This has caused a Regression  Paint increase 15.1% on MacOSX 10.7 Mozilla-Inbound

Was it expected?

Comment 27

[Marco Bonardo [:mak]]

I pinged heycam and decided to backout while the regression is investigated. So this is has been backed out.

Comment 28

[Cameron McCormack (:heycam)]

Markus, how do we proceed?  The regression flagged on mozilla-inbound is 46ms (http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/26110c50e16c2c5c).  It's difficult for me to tell exactly which talos box on tbpl this corresponds to.  It might be tdhtml.2_paint (in the chrome_mac.2 group)?  At least that has a number that's in the 300s like the one with the regression mail.

I put some logging around the changed bit of code in nsCocoaWindow.mm (where it does the makeKeyAndOrderFront call) and there is 10-30ms of extra time there when a window is shown as part of those chrome_mac.2 tests.

Comment 29

[Markus Stange [:mstange]]

Sorry, I have no idea. Looking at https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fb59e7eae2a9 and https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3e7875d6ee51 , the reported regression seems to be about the "tpaint" test on the box called "Rev4 MacOSX Lion 10.7 mozilla-inbound talos chrome_mac". This test apparently doesn't exist on the try server. I haven't been able to find out what this test does and where its code is located, and unfortunately I don't have the time to ask around.

My opinion: If it's only the window creation time that has been regressed (and not general painting, which would be very surprising to me), I'd say the regression is expected. The window animation blocks the user from interacting with the window for a certain time (over 100ms, I'd guess), so I'm actually surprised that it doesn't block the main thread throughout the animation (only for the 10-30ms at the beginning, where it probably sets up the animation and maybe spawns an animation thread).

Comment 30

[Cameron McCormack (:heycam)]

The set of talos tests that get run on inbound must have changed since I landed this.  I'm going to land it again and if we still get a regression hopefully we will see exactly which test it is.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ffb4e09ac4a

Comment 32

[Christopher Tipper]

Maybe not appropriate here, but I am annoyed that Firefox 21 seems to have disabled the double-tap title bar to minimise to dock functionality (which is standard for many applications on 10.7). I am really not happy that there isn't even a user-preference to re-enable the action. Is this anything to do with this fix?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Double-clicking the title bar to minimize works just fine for me in Firefox 21 (on OS 10.8, though).

Comment 34

[Christopher Tipper]

10.8 for me too. It doesn't work. Even Emacs let's you double click the title bar. I can only conclude you haven't tried it. Saying WFM is a pretty lame response if you ask me. I don't think I'll bother next time, maybe just find a replacement browser.

Comment 35

[Christopher Tipper]

(Just to note that I have long disabled tabs-on-top. Stupid feature.)

Comment 36

[Boris Zbarsky [:bzbarsky]]

First of all, I have in fact tried it and it does in fact work just fine in a clean profile in Firefox 21.

Seconf of all, saying WFM doesn't mean that you don't have the problem; it just means that more information is needed on how our configurations differ.

So how about you file a bug on this totally separate problem you're having, cc me, and we figure out exactly what it is that's causing it for you.