596315 - Animate the opening and closing of the Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

The Web Console would look nicer (and more Quake-esque) if it animated when it opened and closed.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached.

Comment 2

[Rob Campbell [:rc] (:robcee)]

I'll file a separate bug to style the background in concrete or granite.

Comment 3

[David Dahl :ddahl]

mental note: we need to file the 'tilde easter egg' bug

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 475142 [details] [diff] [review]
Proposed patch.

Looks good. I like the animation.


+// Possible directions that can be passed to HUDService.animate().
+const ANIMATE_OUT = "ANIMATE_OUT";
+const ANIMATE_IN = "ANIMATE_IN";

Why use strings and not numbers? 0 and 1.

+   * @param (function(nsIDOMEvent) -> void)? aCallback An optional callback to
+   *        execute once the animation finishes.

That should be just @param function aCallback.... explain what parameter is given to the callback, in the description.

+    if (hudBox.animationDisabled && aCallback) {
+      aCallback(null);

Why null? Just invoke aCallback().

+      case ANIMATE_IN:
+        let contentWindow = hudBox.ownerDocument.defaultView;

As was pointed out to me in some other review... this let is confusing inside the switch. It's scoped inside the entire switch, not inside the case. Better use a var.

+    this.HUDBox.style.height = "0px";

height = 0; doesn't this work?

+    -moz-transition: height 350ms;

Perhaps you should make it shorter. Maybe around 150ms?

Otherwise, the patch looks fine. Feedback+ it is.

Comment 5

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 1.1.

New patch addresses feedback and fixes a random orange in the unit tests. Review requested.

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #4)
> Perhaps you should make it shorter. Maybe around 150ms?

350ms is based on advice from Aza and I think it looks best the way it is.

Comment 7

[David Dahl :ddahl]

(In reply to comment #6)
> (In reply to comment #4)
> > Perhaps you should make it shorter. Maybe around 150ms?
> 
> 350ms is based on advice from Aza and I think it looks best the way it is.

If developers think it is slow, this will feature will backfire. Just saying. We need feedback from multiple develoers here.

Comment 8

[David Dahl :ddahl]

In fact, you should take a screencast of it opening at different speeds - say 150, 250 and 350 ms - and ask fx-team and web dev teams what they think.

Comment 9

[Patrick Walton (:pcwalton)]

(In reply to comment #8)
> In fact, you should take a screencast of it opening at different speeds - say
> 150, 250 and 350 ms - and ask fx-team and web dev teams what they think.

I'm not sure if screencasts would help, since this is the kind of thing that needs to be viewed in context. If this patch lands I'll monitor Firefox Input for any comments on the animation speed.

Comment 10

[Rob Campbell [:rc] (:robcee)]

let's bikeshed animation lengths!

150ms is short enough to be barely visible.

the difference between 250 and 350 is very little. .3s is probably about the amount of time it would take for a human to enter the keyboard shortcut and then move onto another key to start typing something. I think it's a fine duration.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.


>+  /**
>+   * Disables all animation for a console, for unit testing. After this call,
>+   * the console will instantly take on a reasonable height, and the close
>+   * animation will not occur.
>+   *
>+   * @param string aHUDId The ID of the console.
>+   */
>+  disableAnimation: function HS_disableAnimation(aHUDId)
>+  {
>+    let hudBox = this.getOutputNodeById(aHUDId);
>+    hudBox.classList.remove("animated");
>+    hudBox.style.height = "300px";
>   }

if that bit is for tests only, should #ifdef if it out for production builds?

>+.hud-box.animated {
>+    -moz-transition: height 350ms;
>+}
>+

hm. i was actually going to file a bug about persisting the console height, because it's terribly annoying that it doesn't remember, when you've opened/closed it a few hundred times an hour. we might want to push this into script, so we can support user-defined height.

or you could do that in another bug. i'm all for getting this in for nightly tester feedback, and then tweaking.

Comment 12

[Kevin Dangoor]

Opened bug 601909 to save the console height.

Comment 13

[Justin Wood (:Callek)]

(In reply to comment #11)
> Comment on attachment 475230 [details] [diff] [review]
> Proposed patch, version 1.1.
> 
> 
> >+  /**
> >+   * Disables all animation for a console, for unit testing. After this call,
> >+   * the console will instantly take on a reasonable height, and the close
> >+   * animation will not occur.
> >+   *
> >+   * @param string aHUDId The ID of the console.
> >+   */
> >+  disableAnimation: function HS_disableAnimation(aHUDId)
> >+  {
> >+    let hudBox = this.getOutputNodeById(aHUDId);
> >+    hudBox.classList.remove("animated");
> >+    hudBox.style.height = "300px";
> >   }
> 
> if that bit is for tests only, should #ifdef if it out for production builds?

If my memory is right, we are moving toward building with --enable-tests on release builds, so that we can actually run the whole testsuite on the builds we ship too.

We currently *do* run tests on opt builds as well. So I think ifdeffing this code out entirely is probably prone to future problems. That said I'm not sure if you want to just keep it or do something else here, dietrich?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

We already build release builds with --enable-tests, and run tests on them.

Comment 15

[Patrick Walton (:pcwalton)]

Going to go ahead and ask for approval on this then.

Comment 16

[Johnathan Nightingale [:johnath]]

Comment on attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.

Approved for landing post-beta7

vooosh

Comment 17

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.

http://hg.mozilla.org/mozilla-central/rev/bdcaec24bbcb

Comment 18

[Patrick Walton (:pcwalton)]

(In reply to comment #17)
> Comment on attachment 475230 [details] [diff] [review]
> [checked-in] Proposed patch, version 1.1.
> 
> http://hg.mozilla.org/mozilla-central/rev/bdcaec24bbcb

I love you.

Comment 19

[Rob Campbell [:rc] (:robcee)]

added

http://hg.mozilla.org/mozilla-central/rev/25df336746e4

to fix merge bustage.