The default bug view has changed. See this FAQ.

Animate the opening and closing of the Web Console

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 ?)

Details

(Whiteboard: [parity-quake][approved-for-landing-post-b7])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 475142 [details] [diff] [review]
Proposed patch.

Proposed patch attached.
Attachment #475142 - Flags: feedback?(mihai.sucan)
Whiteboard: [quakesque]
I'll file a separate bug to style the background in concrete or granite.
(Assignee)

Updated

7 years ago
Whiteboard: [quakesque] → [parity-quake]

Comment 3

7 years ago
mental note: we need to file the 'tilde easter egg' bug
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.
Attachment #475142 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 5

7 years ago
Created attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.

New patch addresses feedback and fixes a random orange in the unit tests. Review requested.
Attachment #475230 - Flags: review?(dietrich)
(Assignee)

Comment 6

7 years ago
(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

7 years ago
(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

7 years ago
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.
(Assignee)

Comment 9

7 years ago
(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.
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.

Updated

7 years ago
Blocks: 594225
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.
Attachment #475230 - Flags: review?(dietrich) → review+

Comment 12

7 years ago
Opened bug 601909 to save the console height.
(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?
status2.0: --- → ?
Version: unspecified → Trunk
We already build release builds with --enable-tests, and run tests on them.
(Assignee)

Comment 15

7 years ago
Going to go ahead and ask for approval on this then.
(Assignee)

Updated

7 years ago
Attachment #475142 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Attachment #475142 - Attachment is obsolete: true
Attachment #475142 - Flags: approval2.0?
(Assignee)

Updated

7 years ago
Attachment #475230 - Flags: approval2.0?
Comment on attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.

Approved for landing post-beta7

vooosh
Attachment #475230 - Flags: approval2.0? → approval2.0+

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [parity-quake] → [parity-quake][approved-for-landing-post-b7]
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 475230 [details] [diff] [review]
[checked-in] Proposed patch, version 1.1.

http://hg.mozilla.org/mozilla-central/rev/bdcaec24bbcb
Attachment #475230 - Attachment description: Proposed patch, version 1.1. → [checked-in] Proposed patch, version 1.1.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 18

7 years ago
(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.
added

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

to fix merge bustage.
You need to log in before you can comment on or make changes to this bug.