Closed Bug 873848 Opened 7 years ago Closed 6 years ago

Make the developer toolbar consistent with Toolbox design. (aka Implement shorlander's developer toolbar design)

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: Optimizer, Assigned: Optimizer)

References

()

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 files, 6 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
This is makes the developer toolbar and gcli appear like what it is in the mockups from http://shorlander.dropmark.com/85185/1111799 and http://people.mozilla.com/~shorlander/files/devTools-toolbox-i02/devTools-toolbox-i02.html

It also cleans up a lot of things (one unused icon, one class, some css from browser.css, some html from browser.xul etc.).

Current behavior that I have : http://people.mozilla.com/~shorlander/files/devTools-toolbox-i02/devTools-toolbox-i02.html

The patch is right now windows aero only.
(In reply to Girish Sharma [:Optimizer] from comment #0)
> Current behavior that I have :
> http://people.mozilla.com/~shorlander/files/devTools-toolbox-i02/devTools-
> toolbox-i02.html

Ugh, I meant http://i.snag.gy/bgMqH.jpg
Attached patch patch v0.1 (obsolete) — Splinter Review
refactored a lot of styles, removed unused png and made some png's unused to remove them.

The previous screenshot has a margin b/w the gcli input textbox and the wrench icon to tell them both apart in the checked state of the wrench button. This patch removes that margin, and it looks like this ( http://i.snag.gy/j3WWk.jpg ) now. I would like Stephen's opinion here, are the two differentiable, should I change the background of the button, etc.

I would also need some help on the wrench icons to make them similar to the paint flashing icon set : chrome://browser/skin/devtools/command-paintflashing.png . The one that I am using here has a weird blue tint. (because of my poor photoshop skills)
Assignee: nobody → scrapmachines
Attachment #751477 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #751806 - Flags: review?(jwalker)
Attachment #751806 - Flags: review?(fayearthur)
Attachment #751806 - Flags: review?(dao)
Flags: needinfo?(shorlander)
Added Dao for the browser side changes in browser.css and browser.xul, Joe for the gcli and toolbar changes and Heather for the overall style changes.
Attachment #751806 - Flags: review?(jwalker) → review+
Comment on attachment 751806 [details] [diff] [review]
patch v0.1

Look who's back.
Attachment #751806 - Flags: review?(fayearthur) → review?(paul)
Comment on attachment 751806 [details] [diff] [review]
patch v0.1

Review of attachment 751806 [details] [diff] [review]:
-----------------------------------------------------------------

On osx, the ">>" icon is missing, and there's an extra padding at the bottom of the toolbar.
Attachment #751806 - Flags: review?(paul) → review-
(In reply to Paul Rouget [:paul] from comment #5)
> Comment on attachment 751806 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 751806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On osx, the ">>" icon is missing, and there's an extra padding at the bottom
> of the toolbar.

Fixed locally. Is everything else okay ?
>diff --git a/browser/themes/linux/browser.css b/browser/themes/linux/browser.css
> #developer-toolbar {
>-  border-top: 1px solid hsla(210, 8%, 5%, .65);
>+  padding: 0;
>+  min-height: 32px;
>+  background-image: url(devtools/background-noise-toolbar.png), linear-gradient(#303840, #2d3640);
>+  border-top: 1px solid #060a0d;
>+  border-bottom: 1px solid transparent;

Is this border useful?

>+  box-shadow: 0 1px 0 hsla(204,45%,98%,.05) inset, 0 -1px 0 hsla(206,37%,4%,.1) inset;
> }
> /* Web Console */
>.
> .web-console-frame {
>   border-bottom: 1px solid #aaa;
> }
>.
> /* Developer Toolbar */

Duplicated comment.

>+.developer-toolbar-button > image {
>+  margin: auto 10px;
>+}

Why "auto"?
We usually use pack and align to center images (if this is the goal).

>diff --git a/browser/themes/linux/devtools/common.css b/browser/themes/linux/devtools/common.css
>--- a/browser/themes/linux/devtools/common.css
>+++ b/browser/themes/linux/devtools/common.css
>@@ -180,26 +180,42 @@
> .devtools-closebutton > .toolbarbutton-icon {
>   /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
>      use evil CSS to give the impression of smaller content */
>   margin: -4px;
> }
>.
>+.devtools-closebutton > .toolbarbutton-text {
>+  display: none;
>+}
>+
>+.devtools-closebutton:hover {
>+  opacity: 0.8;
>+}
>+
>+.devtools-closebutton:hover:active {
>+  opacity: 1;
>+}

I guess if you do that, we don't need to use "toolbox-close" in toolbox.xul,
but just the devtools-closebutton class.
Attached patch patch v0.2 (obsolete) — Splinter Review
Changes in the patch: 
- removed styles related to #toolbox-close and directly using .devtools-closebutton to render the close button.
- moved the develoepr toolbar related stuff form browser.css to commandline.inc.css (shared, single place, outside of browser.css \0/)
- fixed missing commandline-icon.png on osx

reply to Paul's last comment:
>diff --git a/browser/themes/linux/browser.css b/browser/themes/linux/browser.css
> #developer-toolbar {
>-  border-top: 1px solid hsla(210, 8%, 5%, .65);
>+  padding: 0;
>+  min-height: 32px;
>+  background-image: url(devtools/background-noise-toolbar.png), linear-gradient(#303840, #2d3640);
>+  border-top: 1px solid #060a0d;
>+  border-bottom: 1px solid transparent;

Is this border useful?

Nope, and I don't even remember why I put it there now ... removed.

>+  box-shadow: 0 1px 0 hsla(204,45%,98%,.05) inset, 0 -1px 0 hsla(206,37%,4%,.1) inset;
> }
> /* Web Console */
>.
> .web-console-frame {
>   border-bottom: 1px solid #aaa;
> }
>.
> /* Developer Toolbar */

Duplicated comment.

fixed.

>+.developer-toolbar-button > image {
>+  margin: auto 10px;
>+}

Why "auto"?
We usually use pack and align to center images (if this is the goal).

The goal here is to make the wrench button take some extra width with the icon in center. I am not trying to just align the icon in the center, but to give some specific margin so as to meet the mockups.

>diff --git a/browser/themes/linux/devtools/common.css b/browser/themes/linux/devtools/common.css
>--- a/browser/themes/linux/devtools/common.css
>+++ b/browser/themes/linux/devtools/common.css
>@@ -180,26 +180,42 @@
> .devtools-closebutton > .toolbarbutton-icon {
>   /* XXX Buttons have padding in widget/ that we don't want here but can't override with good CSS, so we must
>      use evil CSS to give the impression of smaller content */
>   margin: -4px;
> }
>.
>+.devtools-closebutton > .toolbarbutton-text {
>+  display: none;
>+}
>+
>+.devtools-closebutton:hover {
>+  opacity: 0.8;
>+}
>+
>+.devtools-closebutton:hover:active {
>+  opacity: 1;
>+}

I guess if you do that, we don't need to use "toolbox-close" in toolbox.xul,
but just the devtools-closebutton class.

Great catch. Moar CSS remocal!!!
Attachment #751806 - Attachment is obsolete: true
Attachment #751806 - Flags: review?(dao)
Attachment #759882 - Flags: review?(paul)
Attachment #759882 - Flags: review?(dao)
Attached patch gcli popup changes (obsolete) — Splinter Review
This is second patch on top of patch v0.2
this contains:
- Left out class="devtools-closebutton" for toolbox close button on windows.
- GCLI popup changes to have consistent background color and border color.
Attachment #760080 - Flags: review?(paul)
Why do you remove the ::moz-selection code for Linux, but not for osx?
> -        <toolbarbutton id="toolbox-close"
> +        <toolbarbutton class="devtools-closebutton"

We use this id: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#202

Remove only the CSS code.
The delete button is not very visible in the variable view on osx:

>  .variables-view-delete {
> -  list-style-image: url("chrome://browser/skin/devtools/toolbarbutton-close.png");
> +  list-style-image: url("chrome://browser/skin/tabview/close.png");
>    -moz-image-region: rect(0,32px,16px,16px);
>    opacity: 0;
>  }

Maybe we want a specific icon for that.

Also - please validate the UI changes you made for the debugger and the webconsole with Victor and Mihai.
Comment on attachment 759882 [details] [diff] [review]
patch v0.2

(address previous comments)
Attachment #759882 - Flags: review?(paul) → review-
Attachment #760080 - Flags: review?(paul) → review+
(In reply to Paul Rouget [:paul] from comment #12)
> > -        <toolbarbutton id="toolbox-close"
> > +        <toolbarbutton class="devtools-closebutton"
> 
> We use this id:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> toolbox.js#202
> 
> Remove only the CSS code.

Fixed in second patch.

(In reply to Paul Rouget [:paul] from comment #13)
> The delete button is not very visible in the variable view on osx:
> 
> >  .variables-view-delete {
> > -  list-style-image: url("chrome://browser/skin/devtools/toolbarbutton-close.png");
> > +  list-style-image: url("chrome://browser/skin/tabview/close.png");
> >    -moz-image-region: rect(0,32px,16px,16px);
> >    opacity: 0;
> >  }
> 
> Maybe we want a specific icon for that.

As in ? Earlier also we were using the active state of a close button only.

> 
> Also - please validate the UI changes you made for the debugger and the
> webconsole with Victor and Mihai.

Already done. I changed after taking their permission

(In reply to Paul Rouget [:paul] from comment #11)
> Why do you remove the ::moz-selection code for Linux, but not for osx?

Umm. missed that. So, is the selection color required ? If yes, I will not remove from both places, otherwise, will remove from both places.
> is the selection color required ?

I think it is.
Using the new icon provided by Stephen. He also suggested that we loose the hover/active/checked background colors from the wrench button so that it remains visibly different from the input box of gcli.

Added back the moz-selection colors.
Attachment #759882 - Attachment is obsolete: true
Attachment #760080 - Attachment is obsolete: true
Attachment #759882 - Flags: review?(dao)
Attachment #762874 - Flags: review?(paul)
Attachment #762874 - Flags: review?(dao)
> (In reply to Paul Rouget [:paul] from comment #13)
> > The delete button is not very visible in the variable view on osx:
> > 
> > >  .variables-view-delete {
> > > -  list-style-image: url("chrome://browser/skin/devtools/toolbarbutton-close.png");
> > > +  list-style-image: url("chrome://browser/skin/tabview/close.png");
> > >    -moz-image-region: rect(0,32px,16px,16px);
> > >    opacity: 0;
> > >  }
> > 
> > Maybe we want a specific icon for that.
> 
> As in ? Earlier also we were using the active state of a close button only.

Let's not make our devtools Delete button depend on the look of the Firefox tab close button. It's even more important today because we are redesigning the tabs in Firefox (Australis).

We want a proper icon here.

Beside that, r+
Attachment #762874 - Flags: review?(paul)
Attached patch final patch (obsolete) — Splinter Review
Adding a separate icon for the remove button for debugger's watch list. Basically using the same icon as before, just cropped the icon to have only one of the three states.

Carry forward r+ from Paul
Attachment #762874 - Attachment is obsolete: true
Attachment #762874 - Flags: review?(dao)
Attachment #764361 - Flags: review?(dao)
Blocks: 868163
Comment on attachment 764361 [details] [diff] [review]
final patch

Review of attachment 764361 [details] [diff] [review]:
-----------------------------------------------------------------

I looked over the browser.xul, jar.mn, browser/base/content/browser.css, and browser/themes/*/browser.css and didn't see any issues. I'll trust Paul's review of the devtools specific changes.

::: browser/themes/linux/browser.css
@@ -2097,5 @@
>    border-bottom: 1px solid #aaa;
>  }
>  
> -.web-console-frame[animated] {
> -  transition: height 100ms;

We talked over IRC and this is dead code that is just being removed, but it should be called out in the commit summary since it seems to have nothing to do with the new design for the developer toolbar :)
Attachment #764361 - Flags: review?(dao) → review+
Thanks Jared for the review :)

Added commit message calling out the additional image and css cleanup.
Attachment #764361 - Attachment is obsolete: true
Attachment #764897 - Flags: review+
Whiteboard: [land-in-fx-team]
Blocks: 884887
Landed:
https://hg.mozilla.org/integration/fx-team/rev/c8bec6945251
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c8bec6945251
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.