Closed Bug 581231 Opened 10 years ago Closed 10 years ago

Add the Close button to the bottom of the Console

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(blocking2.0 -)

VERIFIED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- -

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

(Whiteboard: [kd4b6] [patchclean:0908] [Input])

Attachments

(8 files, 5 obsolete files)

2.32 KB, patch
dietrich
: review+
ddahl
: feedback+
Details | Diff | Splinter Review
2.83 KB, patch
dietrich
: review+
ddahl
: feedback+
Details | Diff | Splinter Review
73.11 KB, image/png
Details
4.40 KB, patch
benjamin
: approval2.0+
Details | Diff | Splinter Review
1.74 KB, patch
dietrich
: review+
msucan
: feedback+
dietrich
: approval2.0+
Details | Diff | Splinter Review
2.71 KB, patch
dietrich
: review+
msucan
: feedback+
dietrich
: approval2.0+
Details | Diff | Splinter Review
3.15 KB, patch
dietrich
: approval2.0+
Details | Diff | Splinter Review
4.48 KB, patch
Details | Diff | Splinter Review
Attached patch Proposed patch. (obsolete) — Splinter Review
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.
Attachment #459666 - Flags: review?(gavin.sharp)
Blocks: consoleui
Summary: Surround the Console's input field in a containing box → Add Close and Clear buttons to the bottom of the Console
Updated the description to more accurately reflect the actual user impact of this bug.
Why is the CSS width needed?
Attachment #459666 - Flags: review?(gavin.sharp)
Canceling review due to test failures.
New patch removes the "width: 100%" and applies cleanly on trunk. No test failures.
Attachment #459666 - Attachment is obsolete: true
Attachment #460360 - Flags: review?(gavin.sharp)
Summary: Add Close and Clear buttons to the bottom of the Console → Add the Close button to the bottom of the Console
Attachment #460360 - Flags: feedback?(ddahl)
Attachment #460360 - Flags: feedback?(ddahl) → feedback+
Patch part 2 adds the actual close button and styles it appropriately on the Mac.
Attachment #461735 - Flags: feedback?(ddahl)
Whiteboard: [kd4b4]
Attachment #460360 - Flags: review?(gavin.sharp) → review?(dietrich)
Attachment #461735 - Flags: feedback?(ddahl) → feedback+
Depends on: 584571
Attachment #461735 - Flags: review?(dietrich)
Attachment #460360 - Flags: review?(dietrich) → review+
Comment on attachment 461735 [details] [diff] [review]
Proposed patch, part 2.

Can you attach a screenshot?
Attached image Mac screenshot.
Mac screenshot attached.
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.
Attachment #461735 - Flags: review?(dietrich) → review+
Updated per review comments. UI review requested.
Attachment #463551 - Flags: ui-review?(limi)
Duplicate of this bug: 585530
Wouldn't hold back the release for this, but definitely will approve the fully-reviewed patch.
blocking2.0: ? → -
Keywords: uiwanted
Rebased part 1 to trunk. The changes were minimal.
Rebased part 2. Trivial changes.
Attachment #463551 - Attachment is obsolete: true
Attachment #464661 - Flags: ui-review?(limi)
Attachment #463551 - Flags: ui-review?(limi)
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.
Attachment #464661 - Flags: ui-review?(limi) → ui-review+
Attachment #464661 - Flags: approval2.0?
Whiteboard: [kd4b4] → [kd4b5]
Why is the button only styled on Mac?
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.
Duplicate of this bug: 588349
Attachment #464661 - Flags: approval2.0? → approval2.0+
patch part 1, version 2 has bitrotted (according to pwalton) and part 2 has not been tested.
Whiteboard: [kd4b5] → [kd4b5] [patchbitrot]
Whiteboard: [kd4b5] [patchbitrot] → [kd4b6] [patchbitrot]
Patch part 1 rebased to trunk.
Attachment #464659 - Attachment is obsolete: true
Patch part 2 rebased to trunk.
Patch part 3 adds Windows and Linux support.
Attachment #472021 - Flags: feedback?(ddahl)
Patch part 4 adds a unit test. This completes the patch series for this bug.
Attachment #472039 - Flags: feedback?(ddahl)
Whiteboard: [kd4b6] [patchbitrot] [Input] → [kd4b6] [patchclean:0903] [Input]
Blocks: devtools4b7
Attachment #472021 - Flags: feedback?(ddahl) → feedback?(mihai.sucan)
Attachment #472039 - Flags: feedback?(ddahl) → feedback?(mihai.sucan)
Attachment #472021 - Flags: review?(dietrich)
Attachment #472039 - Flags: review?(dietrich)
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?
Attachment #472021 - Flags: feedback?(mihai.sucan) → feedback+
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.
Attachment #472039 - Flags: feedback?(mihai.sucan) → feedback+
Comment on attachment 472021 [details] [diff] [review]
Proposed patch, part 3.

why all the !important? and why no mac styling?
Comment on attachment 472039 [details] [diff] [review]
Proposed patch, part 4.

r=me with mihai's comments addressed.
Attachment #472039 - Flags: review?(dietrich) → review+
(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.
Turns out the !important is actually all necessary.
Whiteboard: [kd4b6] [patchclean:0903] [Input] → [kd4b6] [patchbitrot] [Input]
Patch part 1 version 2.1 rebases to trunk.
Proposed patch part 2, version 1.1 rebases to trunk.
Whiteboard: [kd4b6] [patchbitrot] [Input] → [kd4b6] [patchclean:0908] [Input]
Attachment #472021 - Flags: review?(dietrich)
Attachment #472021 - Flags: review+
Attachment #472021 - Flags: approval2.0+
Attachment #472039 - Flags: approval2.0?
Attachment #460360 - Flags: approval2.0?
Attachment #461735 - Flags: approval2.0?
Attachment #461735 - Flags: approval2.0?
Attachment #460360 - Flags: approval2.0?
Attachment #470550 - Attachment is obsolete: true
Attachment #470551 - Attachment is obsolete: true
Attachment #473119 - Flags: approval2.0?
Attachment #472039 - Flags: approval2.0? → approval2.0+
Attachment #473119 - Flags: approval2.0? → approval2.0+
Keywords: uiwantedcheckin-needed
was this tested on try?/windows/linux?
Try no, Windows yes, Linux yes.
Keywords: checkin-needed
in today's build, 2010-09-09.

it is beautiful.
Status: RESOLVED → VERIFIED
Depends on: 595106
No longer depends on: 595106
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.