581231 - Add the Close button to the bottom of the Console

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

The Console's input field needs to be surrounded in a containing hbox so that the Close and Clear buttons can be added there. See the mockups in bug 559481. This patch implements this.

This patch is part of the Firefox 4 Console UI. The current UI for the console
is simply a placeholder and has not yet been designed from a UX perspective
(see bug 559481). Because I am requesting blocking status for the Console UI,
and this is a critical part of the Console user experience, I am requesting
that this bug block the Firefox 4 release as well.

Comment 1

[Patrick Walton (:pcwalton)]

Updated the description to more accurately reflect the actual user impact of this bug.

Comment 2

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

Why is the CSS width needed?

Comment 3

[Patrick Walton (:pcwalton)]

Canceling review due to test failures.

Comment 4

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2.

New patch removes the "width: 100%" and applies cleanly on trunk. No test failures.

Comment 5

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2.

Patch part 2 adds the actual close button and styles it appropriately on the Mac.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 461735 [details] [diff] [review]
Proposed patch, part 2.

Can you attach a screenshot?

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Mac screenshot.

Mac screenshot attached.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 461735 [details] [diff] [review]
Proposed patch, part 2.

>+    let closeButton = this.xulElementFactory("button");
>+    closeButton.setAttribute("class", "jsterm-close-button");
>+    inputContainer.appendChild(closeButton);
>+    closeButton.addEventListener("command", HeadsUpDisplayUICommands.toggleHUD,
>+      false);

line up indent with other params

>diff --git a/toolkit/themes/pinstripe/global/icons/console-close.png b/toolkit/themes/pinstripe/global/icons/console-close.png
>new file mode 100644

please get UI-R+ on the icon before adding.

r=me. please request approval to land after getting UI-R on the new icon.

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2 (revised per review)

Updated per review comments. UI review requested.

Comment 11

[Dietrich Ayala (:dietrich)]

Wouldn't hold back the release for this, but definitely will approve the fully-reviewed patch.

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2 (trunk rebase 2010-08-10).

Rebased part 1 to trunk. The changes were minimal.

Comment 13

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2 (revised per review, trunk rebase 2010-08-10).

Rebased part 2. Trivial changes.

Comment 14

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

Comment on attachment 464661 [details] [diff] [review]
Proposed patch, part 2 (revised per review, trunk rebase 2010-08-10).

Much improved over the previous UI iteration.

It is a bit dangerous to put this control where there would normally be an "Evaluate" or "Execute" button that acts on the text input, but as long as there's no data loss involved, this is certainly better.

Another approach would be to have the close button be inside the console itself (where the output is), but I'd have to explore how to make this not be messy if we were to do such a thing.

Comment 15

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

Why is the button only styled on Mac?

Comment 16

[Patrick Walton (:pcwalton)]

I have styling for Windows and Linux, but the patches have bitrotted. Been focused on patches that need to get in before string freeze; will clean them up before betaN.

Comment 18

[Kevin Dangoor]

patch part 1, version 2 has bitrotted (according to pwalton) and part 2 has not been tested.

Comment 19

[Aakash Desai [:aakashd]]

Driveby Input search for users sending feedback about this issue - http://input.mozilla.com/en-US/search/?version=&product=firefox&date_start=&date_end=&sentiment=sad&q=tool+close

http://input.mozilla.com/en-US/search/?product=firefox&sentiment=sad&date_end=&date_start=&version=&q=console+close

Comment 20

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2.1.

Patch part 1 rebased to trunk.

Comment 21

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2.1.

Patch part 2 rebased to trunk.

Comment 22

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3.

Patch part 3 adds Windows and Linux support.

Comment 23

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4.

Patch part 4 adds a unit test. This completes the patch series for this bug.

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 472021 [details] [diff] [review]
Proposed patch, part 3.

This looks fine to me. However, I am not sure why you do not use background-image instead of list-style-image? Also, why do you have all those !important priorities? Is there another stylesheet overwriting those properties?

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 472039 [details] [diff] [review]
Proposed patch, part 4.

The test looks fine. However, the tabBrowser variable name is a misnomer. gBrowser is the tabbrowser (see tabbrowser.xul). Also, instead of using getBrowserForTab() you can directly use gBrowser.selectedBrowser.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 472021 [details] [diff] [review]
Proposed patch, part 3.

why all the !important? and why no mac styling?

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 472039 [details] [diff] [review]
Proposed patch, part 4.

r=me with mihai's comments addressed.

Comment 28

[Patrick Walton (:pcwalton)]

(In reply to comment #26)
> Comment on attachment 472021 [details] [diff] [review]
> Proposed patch, part 3.
> 
> why all the !important? and why no mac styling?

Mac styling is in patch part 2. I'm currently in the process of taking an axe to as much of the !important as I can.

Comment 29

[Patrick Walton (:pcwalton)]

Turns out the !important is actually all necessary.

Comment 30

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2.1.

Patch part 1 version 2.1 rebases to trunk.

Comment 31

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 1.1.

Proposed patch part 2, version 1.1 rebases to trunk.

Comment 32

[David Dahl :ddahl]

was this tested on try?/windows/linux?

Comment 33

[Patrick Walton (:pcwalton)]

Try no, Windows yes, Linux yes.

Comment 34

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/976d2a371fe3
http://hg.mozilla.org/mozilla-central/rev/4836972f8f75
http://hg.mozilla.org/mozilla-central/rev/d207a06a5d52
http://hg.mozilla.org/mozilla-central/rev/6b33742b5900

Comment 35

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

in today's build, 2010-09-09.

it is beautiful.

Comment 36

[Mihai Sucan [:msucan]]

From the test code:

  EventUtils.synthesizeMouse(closeButton, 0, 0, {});

  ok(!(hudId in HUDService.windowRegistry), "the console is closed when the " +
     "close button is pressed");

In opt builds, the test passes, but it fails for me in debug builds. I believe the code should wait for the click event before checking.