Closed Bug 674370 Opened 13 years ago Closed 12 years ago

[10.7] Support animation when opening windows in Lion

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: limi, Assigned: heycam)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [parity-safari])

Attachments

(1 file, 4 obsolete files)

When opening the application on OS X Lion, we should use the same animation as the other applications. (Zoom in)
Summary: Support animation when opening the application in Lion → [10.7] Support animation when opening the application in Lion
Actually the animation happens when opening any window.
Summary: [10.7] Support animation when opening the application in Lion → [10.7] Support animation when opening windows in Lion
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attached file Minimal WIP v0.1 (obsolete) —
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.
Assignee: cam → nobody
Component: General → Widget: Cocoa
Product: Firefox → Core
QA Contact: general → cocoa
Assignee: nobody → cam
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?
Josh, do you have an opinion on my comment 3?
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.
Attached patch WIP v0.2 (obsolete) — Splinter Review
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
Attachment #577865 - Attachment is obsolete: true
Oh and the Scratchpad window, since you can have multiple of those open, and they look fairly document like.
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.
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 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.
Attachment #584920 - Flags: review?(joshmoz) → review?(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.
(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).
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)?
Target Milestone: --- → mozilla12
Target Milestone: mozilla12 → ---
Sure.
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.
Attachment #594590 - Flags: review?(mstange)
Attachment #584920 - Attachment is obsolete: true
Attachment #584920 - Flags: review?(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!
Attachment #594590 - Flags: review?(mstange) → review+
(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!
Attachment #594590 - Flags: superreview?(dao)
Boris, can you superreview my chrome flag changes here?
Attachment #594590 - Flags: superreview?(dao)
Attachment #594590 - Flags: superreview?(bzbarsky)
Attachment #594590 - Flags: review?(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.
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)?
(Modulo extreme typos of course.)
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!)
Attachment #594590 - Attachment is obsolete: true
Attachment #594590 - Flags: superreview?(bzbarsky)
Attachment #594590 - Flags: review?(dao)
Attachment #594879 - Flags: superreview?(bzbarsky)
Attachment #594879 - Flags: review?(mstange)
Attachment #594879 - Flags: review?(dao)
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.
Attachment #594879 - Flags: superreview?(bzbarsky) → superreview+
Attachment #594879 - Flags: review?(mstange) → review+
Attachment #594879 - Flags: review?(dao) → review+
This has caused a Regression  Paint increase 15.1% on MacOSX 10.7 Mozilla-Inbound

Was it expected?
I pinged heycam and decided to backout while the regression is investigated. So this is has been backed out.
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.
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).
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.
https://hg.mozilla.org/mozilla-central/rev/2ffb4e09ac4a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 728493
Depends on: 736334
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?
Double-clicking the title bar to minimize works just fine for me in Firefox 21 (on OS 10.8, though).
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.
(Just to note that I have long disabled tabs-on-top. Stupid feature.)
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.
Depends on: 740008
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: