942292 - DevTools themes - make the toolbars thinner

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

See https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail] and https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail] for examples.  They are vertically shorter, and have a flatter design.

Comment 1

[Tim Nguyen :ntim]

I think bug 929127 made the toolbars flat (and fixed partially this bug).

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to ntim007 from comment #1)
> I think bug 929127 made the toolbars flat (and fixed partially this bug).

Bug 929127 was dealing with the sidebar tabs - this is dealing with the whole toolbar row.  There aren't going to need to be a ton of changes here, but there will be changes.

Comment 3

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to ntim007 from comment #1)
> > I think bug 929127 made the toolbars flat (and fixed partially this bug).
> 
> Bug 929127 was dealing with the sidebar tabs - this is dealing with the
> whole toolbar row.  There aren't going to need to be a ton of changes here,
> but there will be changes.

The code from the patch included the flattened toolbar style ( attachment 8335477 [details] [diff] [review] ).
>-  background-image: url(background-noise-toolbar.png), linear-gradient(#3e4750, #3e4750);
>+  background-color: #343c45;
>   border-bottom: 1px solid #060a0d;
>-  box-shadow: 0 1px 0 hsla(204,45%,98%,.05) inset, -1px 0 0 hsla(204,45%,98%,.05) inset, 0 -1px 0 hsla(204,45%,98%,.05) inset;

Comment 4

[Tim Nguyen :ntim]

Maybe this bug could just be for the toolbar buttons and the code mirror find/go to line input.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to ntim007 from comment #4)
> Maybe this bug could just be for the toolbar buttons and the code mirror
> find/go to line input.

The key change here is to shrink the toolbar height by a few px to match the designs, and making sure this is consistently applied across all panels.  Once Bug 941673 lands, I will begin working on this.

Not sure exactly what you mean about the CodeMirror find / go to line, but any CodeMirror-specific design changes should be opened as a separate bug.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbar.patch

Shrinks vertical height on toolbar.  Also updates button design to be more flat, but not fully matching the designs in Comment 1.  There are going to need to be functionality changes to get the console toolbar to work in this manner.

Comment 7

[Girish Sharma [:Optimizer]]

Can we have some screenshots please ? :)

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: console-toolbar-updated

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: debugger-toolbars-updated

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #7)
> Can we have some screenshots please ? :)

Sure, I've uploaded a couple.  Basically, for the buttons, I got rid of the background gradients and box shadows and used the middle-ish color from each gradient instead.  This isn't that close to the final comps, but I think it is a step towards them - I'm not sure if it will stick or if we will just want to go with the lower padding for this bug.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8349000 [details] [diff] [review]
theme-toolbar.patch

Darrin, when this build is complete can you download an executable from the build directory and give some feedback?  There are also a couple of screenshots on this bug if you want to take a look at these, but the button states can be hard to get a good feel for until you click around / hover etc.  Here is the build: https://tbpl.mozilla.org/?tree=Try&rev=d02343b1032a

Comment 12

[Paul Rouget [:paul]]

Can you make it so the toolbars have no padding and inner items no margin?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #12)
> Can you make it so the toolbars have no padding and inner items no margin?

If you look at the designs, I believe that there is a bit of vertical padding on the toolbar (https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]).  See above and below the text input and buttons.  I have it set to 1px right now.  What do you mean exactly by inner items having no margin?

Comment 14

[Paul Rouget [:paul]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Paul Rouget [:paul] from comment #12)
> > Can you make it so the toolbars have no padding and inner items no margin?
> 
> If you look at the designs, I believe that there is a bit of vertical
> padding on the toolbar
> (https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]).  See above and below the text input
> and buttons.  I have it set to 1px right now.  What do you mean exactly by
> inner items having no margin?

I'll formulate that in a different way: can you make it so that the tool toolbar is 22px height, like in the mockups?

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: theme-slimmer-console.png

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: theme-slimmer-debugger.png

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: theme-slimmer-inspector.png

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: theme-toolbar.patch

Darrin, this build will be available at: https://tbpl.mozilla.org/?tree=Try&rev=249692d5003e, or you can have a look at any of the 'slimmer-*' images attached to this bug for a preview of what it looks like.  Basically, I lowered the amount of height on the toolbars, and made it apply more consistently across a few of the panels.  (Comment 10 and Comment 11 are still relevant with this patch).

Comment 19

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Created attachment 8349663 [details]
> theme-slimmer-inspector.png

I must say that until the top and bottom margins from the buttons are not removed, such a small height looks awkward.

Comment 20

[Brian Grinstead [:bgrins]]

Optimizer has also pointed out that the image icons are not square anymore (wider than they are tall).  We could always tweak the existing patch (put a couple more px padding back into the buttons to make it less cramped and make them square again).

Or we could wait until the buttons / breadcrumbs are reskinned before proceeding with this.  For the button designs, we will need to a mockup with designs for different states like hover, active, checked, active / checked, etc.  There is also an option of using the same styles that we have for the command buttons in the tab bar for icon buttons (as opposed to what is there for the console in https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail])

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Optimizer has also pointed out that the image icons are not square anymore
> (wider than they are tall).  We could always tweak the existing patch (put a

To be more clear, What I meant is that the buttons with icons like the debugger stepping buttons (as compared to text buttons in console toolbar) are no longer square. If we go ahead and remove the padding from the toolbar, such that the top bottom borders of the button touches the top bottom border of the toolbar, that they will be square again.


> couple more px padding back into the buttons to make it less cramped and
> make them square again).
> 
> Or we could wait until the buttons / breadcrumbs are reskinned before
> proceeding with this.  For the button designs, we will need to a mockup with
> designs for different states like hover, active, checked, active / checked,
> etc.  There is also an option of using the same styles that we have for the
> command buttons in the tab bar for icon buttons (as opposed to what is there
> for the console in
> https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail])

Yes, I think I saw some mockup, (previously made) which had the same styles as the tabs toolbar buttons on the right, applied to all of the buttons. But maybe I am mistaken.

Comment 22

[Paul Rouget [:paul]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Or we could wait until the buttons / breadcrumbs are reskinned before
> proceeding with this.

Yes, I think this is the way to go.

Comment 23

[Darrin Henein [:darrin]]

> Or we could wait until the buttons / breadcrumbs are reskinned before
> proceeding with this.

I agree with Paul, I think we need the updated buttons and breadcrumbs for such a slim toolbar to work. What do you need from me to make this work?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Darrin Henein [:darrin] from comment #23)
> > Or we could wait until the buttons / breadcrumbs are reskinned before
> > proceeding with this.
> 
> I agree with Paul, I think we need the updated buttons and breadcrumbs for
> such a slim toolbar to work. What do you need from me to make this work?

If I remember correctly, we may need new icons for the breadcrumb separators.  The breadcrumb design is in Bug 936421.  I will open a new bug for the button designs - for this if you can come up with a design showing all the various button states (on both a text button and an icon button) that should be enough to get started.

Comment 25

[Tim Nguyen :ntim]

The slim toolbars look horrible with the current buttons, as mentioned in previous comments, we should probably wait the new toolbar buttons and the breadcrumbs first.

Comment 26

[Brian Grinstead [:bgrins]]

Attachment: thinner-toolbars.patch

Rebased and simpler version of the patch.  Will still need to wait until new buttons land for this to look right

Comment 27

[Darrin Henein [:darrin]]

Attachment: small-toolbars.png

Some ideas on how buttons and various controls can fit inside the thinner toolbars.

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-WIP-part1.patch

Consistently applies height and paddings across toolbars and sidebar tabs

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-WIP-part2.patch

Styles toolbar buttons to be borderless and get close to looking like Attachment 8385335 [details].  Still some questions that have come up regarding styling text only buttons with borders, text only buttons next to icon only buttons.

Comment 31

[Brian Grinstead [:bgrins]]

Binaries of WIP patches: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eea7b2165c0b/

Comment 32

[Tim Nguyen :ntim]

I tried the patches, and it looks quite bad in my opinion, it'd be nice if it actually matches shorlander's mockup.

Comment 33

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-WIP-part2.patch

Another updated patch - this time covering some of the odd cases with text buttons that float in the middle of a panel (such as 'edit and resend' on net panel), and also makes the buttons on the right of the debugger act like the others with hover and opened styling.  I'm not happy with the colors for the different button states in the dark theme, but I think it is ready for ui review anyway.  Pushed to try for a build: https://tbpl.mozilla.org/?tree=Try&rev=f32c0bc071e7.

Comment 34

[Tim Nguyen :ntim]

Here's the styling I'm suggesting : http://jsfiddle.net/ntim/bp7uV/

Based off both shorlander's and dhenein's mockups.

Comment 35

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part1.patch

Comment 36

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

Comment 37

[Victor Porof [:vporof][:vp]]

Comment on attachment 8431139 [details] [diff] [review]
thin-toolbar-part1.patch

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

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +355,4 @@
>    -moz-padding-start: 6px;
>    margin: 0;
>    min-width: 78px;
> +  /* match height to devtools-toolbar - 1px for border */

While I'm much happier now because all these calculations were removed, I wonder if we could go one step further and use box-sizing: border-box where appropriate, to avoid stuff like "1px for border".

Not a review comment, just sharing my thoughts.

::: browser/themes/shared/devtools/widgets.inc.css
@@ +146,5 @@
>  #breadcrumb-separator-before,
>  #breadcrumb-separator-after,
>  #breadcrumb-separator-normal {
>    width: 12px;
> +  height: 24px;

I would also love to see values like "24px" as CSS variables. Definitely followup material.

Comment 38

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #37)
> Comment on attachment 8431139 [details] [diff] [review]
> thin-toolbar-part1.patch
> 
> Review of attachment 8431139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +355,4 @@
> >    -moz-padding-start: 6px;
> >    margin: 0;
> >    min-width: 78px;
> > +  /* match height to devtools-toolbar - 1px for border */
> 
> While I'm much happier now because all these calculations were removed, I
> wonder if we could go one step further and use box-sizing: border-box where
> appropriate, to avoid stuff like "1px for border".

Hmm, looking at that line in particular it seems the comment is inaccurate or that the value should be 23px.

> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +146,5 @@
> >  #breadcrumb-separator-before,
> >  #breadcrumb-separator-after,
> >  #breadcrumb-separator-normal {
> >    width: 12px;
> > +  height: 24px;
> 
> I would also love to see values like "24px" as CSS variables. Definitely
> followup material.

Yes! This would be awesome for a theme to be able to customize toolbar heights

Comment 39

[Brian Grinstead [:bgrins]]

I think the code is actually relatively close to how it will land, even with any UI changes, so I'm going to start trying to get this reviewed.

Here is a screencast of it in action: https://www.youtube.com/watch?v=B_eB2RdMtBI

Comment 40

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #39)
> I think the code is actually relatively close to how it will land, even with
> any UI changes, so I'm going to start trying to get this reviewed.
> 
> Here is a screencast of it in action:
> https://www.youtube.com/watch?v=B_eB2RdMtBI

It'd be nice if you could experiment with expanding separators between button groups. Kinda like the bookmarks menu button in the nav bar, or the menu panel footer.

Comment 41

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part1.patch

This uses border-box sizing on devtools-toolbar to get rid of any ambiguity with the border included in the height.  With just this patch applied, the buttons will look weird, but the height between toolbars, sidebar tabs, and network panel headers should all be consistent (and much smaller than it is currently).

Comment 42

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

UI is still up for discussion, but here are the code changes that have gotten it to the state as seen in the video in Comment 39. 

Darrin, binaries can be grabbed from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5001db24a589/try-macosx64/ if you'd like to have a look

Comment 43

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #42)
> Created attachment 8431563 [details] [diff] [review]
> thin-toolbar-part2.patch
> 
> UI is still up for discussion, but here are the code changes that have
> gotten it to the state as seen in the video in Comment 39. 
> 

In particular, I'm not happy with how text only buttons look.  With the left/right borders and no top/bottom they kind of look like tabs (see style editor), but if there are no borders they absolutely do not look like buttons and would be very hard to discover.  I wonder if we should handle text buttons differently than icon buttons - adding all 4 borders and a px or 2 of vertical padding.

Comment 44

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

Rebased and trying something a bit different for text buttons.  I've also refactored out the big 'reload' button that is used in Canvas, Shader, and Web Audio panels into a [big] button that includes min-height plus some additional margins on the right.

Here is a screencast walking through the UI: https://www.youtube.com/watch?v=2GRzK9UzlkM.  Of course, I'd like UI feedback but again I think the code is pretty close to how it will be regardless of changes so it should be fine to review.

Comment 45

[Tim Nguyen :ntim]

Small mockup I created : http://jsfiddle.net/ntim/bp7uV/
What's great is that it brings out the checked state well.

Comment 46

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

Rebased and updated UI after working with Darrin on it.  Victor, I also added a separator in the debugger between the prettify button and toggle breakpoints as discussed to more clearly show that grouping.

Comment 47

[Tim Nguyen :ntim]

Just tried the patches. I think the buttons are a bit too faint. Can you try to add a border around the buttons that have a background ? Maybe the splitter color but with 0.2 opacity and a 2px border-radius ?

Also, I noticed that the debugger pretty print button has a background, and doesn't have a checked state (I don't see the separator between the toggle breakpoints and prettify buttons btw).

Comment 48

[Tim Nguyen :ntim]

I also noticed that the "Reload" buttons in the center don't have a border, which make them look weird.
Those buttons also have a focus state (a border) which is quite strange.

Comment 49

[Brian Grinstead [:bgrins]]

> Also, I noticed that the debugger pretty print button has a background, and
> doesn't have a checked state (I don't see the separator between the toggle
> breakpoints and prettify buttons btw).

> I also noticed that the "Reload" buttons in the center don't have a border,
> which make them look weird.
> Those buttons also have a focus state (a border) which is quite strange.

Hmm, these must be issues on Windows.  I'm not seeing these problems locally.  Could definitely be something lost in the refactoring - I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0bcc21cb2a15

Comment 50

[Victor Porof [:vporof][:vp]]

Comment on attachment 8437023 [details] [diff] [review]
thin-toolbar-part2.patch

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

Should address some of these observations, but otherwise this looks really good!

1. Certain scope headers in the variables view don't have a bottom border [0].
2. Text and arrows don't appear to be vertically aligned anymore [0] (see "Function scope").
3. The "Sources", "Call Stack", "Variables" etc. toolbar buttons are disproportionately tall vs. the every other toolbar and toolbarbutton.
4. In the network monitor, the table header is a few pixels taller than the tabs in the inspection pane [1].
5. In the webconsole, the logs toolbar is a few pixels taller than the object inspector search toolbar [2].
6. The autocompletion popup is the only remaining UI element with rounded borders [3]. Followup material, but I really think we ought to remove the border radius there.

::: browser/devtools/canvasdebugger/canvasdebugger.xul
@@ +48,5 @@
>              pack="center"
>              flex="1">
>          <button id="reload-notice-button"
>                  class="devtools-toolbarbutton"
> +                big="true"

IMHO this is a much too generic attribute name, and doesn't really tell me why it's needed. Maybe another more descriptive class would be more appropriate?

::: browser/devtools/debugger/debugger-view.js
@@ +537,1 @@
>        button.setAttribute("tooltiptext", this._collapsePaneString);

It's weird having both pane-opened and pane-collapsed attributes. Why not use a "checked" attribute, or "open"? Better yet, why not use just a single one everywhere?

::: browser/devtools/debugger/debugger.xul
@@ +362,5 @@
>                                 tooltiptext="&debuggerUI.sources.blackBoxTooltip;"
>                                 command="blackBoxCommand"/>
>                  <toolbarbutton id="pretty-print"
>                                 class="devtools-toolbarbutton devtools-monospace"
> +                               actlikeimage="true"

A text-as-image attribute sounds better to me

::: browser/devtools/framework/toolbox.xul
@@ +69,5 @@
>      <toolbar class="devtools-tabbar">
>        <hbox id="toolbox-picker-container" />
>        <hbox id="toolbox-tabs" flex="1" />
>        <hbox id="toolbox-buttons" pack="end"/>
> +      <vbox id="toolbox-controls-separator" class="devtools-separator"/>

Why do we now have both toolbox-controls-separator and devtools-separator? Why do we need this distinction?

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +46,3 @@
>  }
>  
> +.devtools-toolbarbutton[label][big] {

Hmm, I don't see why a .devtools-toolbar ancestor is specified above, and not here (nor below). Either enforce a .devtools-toolbar ancestor everywhere, or don't, otherwise things get weird and inconsistent.

Comment 51

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [0]

Comment 52

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [1]

Comment 53

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [2]

Comment 54

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [3]

Comment 55

[Victor Porof [:vporof][:vp]]

Comment on attachment 8431559 [details] [diff] [review]
thin-toolbar-part1.patch

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

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +74,5 @@
>  .requests-menu-header-button {
>    -moz-appearance: none;
>    background: none;
>    min-width: 1px;
> +  min-height: 24px;

See my previous observation in the other review on how the netmonitor toolbars are of different heights.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +16,3 @@
>    border-bottom-width: 1px;
>    border-bottom-style: solid;
> +  min-height: 24px;

Have you tested this height with different font sizes, especially on Linux? How about when using 200% UI elements and/or font-size on Windows? Would 1em be better in this case than a fixed pixel size?

::: browser/themes/shared/devtools/widgets.inc.css
@@ +61,5 @@
>  
>  .breadcrumbs-widget-container {
>    -moz-margin-end: 3px;
> +  max-height: 24px; /* Set max-height for proper sizing on linux */
> +  height: 24px; /* Set height to prevent starting small waiting for content */

Wouldn't just setting height be sufficient? Is height + max-height required? Weird, maybe some flex needs to be removed or a align/pack attribute added to a parent element instead.

Comment 56

[Victor Porof [:vporof][:vp]]

Comment on attachment 8437023 [details] [diff] [review]
thin-toolbar-part2.patch

This was supposed to be a f+, not a f?

Comment 57

[Brian Grinstead [:bgrins]]

Comment on attachment 8437023 [details] [diff] [review]
thin-toolbar-part2.patch

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

::: browser/devtools/canvasdebugger/canvasdebugger.xul
@@ +48,5 @@
>              pack="center"
>              flex="1">
>          <button id="reload-notice-button"
>                  class="devtools-toolbarbutton"
> +                big="true"

I seem to remember a mozilla css style guide somewhere recommending to use attributes instead of class names for this type of thing, but I'm happy to switch it to a class name.  I couldn't really think of a better name at the time - it is bigger than a normal button and has borders.

Maybe devtools-standalone-button or just devtools-button or something like that?  The only issue is that may imply that you don't *also* need to devtools-toolbarbutton class applied (which you currently do).

Thinking about it more, I guess I could make it .devtools-button and remove the devtools-toolbarbutton class altogether

::: browser/devtools/debugger/debugger.xul
@@ +362,5 @@
>                                 tooltiptext="&debuggerUI.sources.blackBoxTooltip;"
>                                 command="blackBoxCommand"/>
>                  <toolbarbutton id="pretty-print"
>                                 class="devtools-toolbarbutton devtools-monospace"
> +                               actlikeimage="true"

Agreed

::: browser/devtools/framework/toolbox.xul
@@ +69,5 @@
>      <toolbar class="devtools-tabbar">
>        <hbox id="toolbox-picker-container" />
>        <hbox id="toolbox-tabs" flex="1" />
>        <hbox id="toolbox-buttons" pack="end"/>
> +      <vbox id="toolbox-controls-separator" class="devtools-separator"/>

toolbox-controls-separator is accessed by ID from js (in toolbox.js).  So devtools-separator is the class to use if you want a separator element.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +46,3 @@
>  }
>  
> +.devtools-toolbarbutton[label][big] {

Buttons that are children of the devtools-toolbar do not have borders, while the [big] ones do (and may or may not live underneath a toolbar).

Comment 58

[Tim Nguyen :ntim]

(In reply to Victor Porof [:vporof][:vp] from comment #50)
> Comment on attachment 8437023 [details] [diff] [review]
> thin-toolbar-part2.patch
> 
> Review of attachment 8437023 [details] [diff] [review]:
> -----------------------------------------------------------------

> 6. The autocompletion popup is the only remaining UI element with rounded
> borders [3]. Followup material, but I really think we ought to remove the
> border radius there.
The tooltip widget and the search box still have rounded corners.

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8431559 [details] [diff] [review]
thin-toolbar-part1.patch

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

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +74,5 @@
>  .requests-menu-header-button {
>    -moz-appearance: none;
>    background: none;
>    min-width: 1px;
> +  min-height: 24px;

Looks like this issue was introduced in part 2

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +16,3 @@
>    border-bottom-width: 1px;
>    border-bottom-style: solid;
> +  min-height: 24px;

I will check on Linux.  Pixel sizes seem to zoom quite well these days - and it makes it easier for consistency with units across other files (and for things like the buttons and breadcrumbs that use some pixel offset).  Before this patch, the toolbar's height was implied by containing elements that were set with px height which seems like it would have the same issues

::: browser/themes/shared/devtools/widgets.inc.css
@@ +61,5 @@
>  
>  .breadcrumbs-widget-container {
>    -moz-margin-end: 3px;
> +  max-height: 24px; /* Set max-height for proper sizing on linux */
> +  height: 24px; /* Set height to prevent starting small waiting for content */

This was due to an issue on Linux (https://bugzilla.mozilla.org/show_bug.cgi?id=936421#c29).  I wasn't able to find another solution at the time, but surely there is something else that could be done

Comment 60

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

OK, I've updated the heights to be more consistent, and renamed [actlikeimage] to [text-as-image] and renamed [big] to [standalone].  I've also added support for [standalone] buttons that are icon buttons (like the snapshot button in the canvas debugger).  Also, removed pane-opened as it wasn't needed after all.

I've pushed to try to test on Linux.  Windows actually looked pretty good from my last build: https://tbpl.mozilla.org/?tree=Try&rev=97923d9ffd03.

Inline feedback below:

> 1. Certain scope headers in the variables view don't have a bottom border
> [0].

I'm not seeing this on http://fiddle.jshell.net/bgrins/kCSDW/show/.  Maybe one of the fixes below picked this up?  Let me know if you are still seeing this

> 2. Text and arrows don't appear to be vertically aligned anymore [0] (see
> "Function scope").

I'm not really seeing it well, but Bug 949462 is swapping out those twistys so maybe that will update this.

> 3. The "Sources", "Call Stack", "Variables" etc. toolbar buttons are
> disproportionately tall vs. the every other toolbar and toolbarbutton.

Fixed by removing a .devtools-sidebar-tabs > tabs > tab selector in debugger.inc.css

> 4. In the network monitor, the table header is a few pixels taller than the
> tabs in the inspection pane [1].

Fixed by removing an extra px I had missed when adding margins to the toolbarbuttons

> 5. In the webconsole, the logs toolbar is a few pixels taller than the
> object inspector search toolbar [2].

Fixed by removing a .variables-view-searchinput selector in widgets.inc.css.

Comment 61

[Brian Grinstead [:bgrins]]

Attachment: toolbars-ubuntu.png

A comparison of the toolbox with and without the patches applied on Ubunutu

Comment 62

[Victor Porof [:vporof][:vp]]

Had a few rejects trying this out. I fixed them myself, but just letting you know.
Hunk #1 FAILED at 28
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/webaudioeditor.inc.css.rej
patching file browser/themes/shared/devtools/widgets.inc.css
Hunk #1 FAILED at 872
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/widgets.inc.css.rej

Also, make sure you synchronize with bug 984051, which adds a "standalone" button to the netmonitor.

Comment 63

[Victor Porof [:vporof][:vp]]

Comment on attachment 8437904 [details] [diff] [review]
thin-toolbar-part2.patch

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

Just a few additional comments:

* Is it just me, or does it look like the text inside the breadcrumbs buttons isn't vertically centered? Compared to the toolbar tabs, it's 1 or 2 px lower than the baseline in the tabs.
* The @media toolbar in the Style Editor has a weirdly placed label inside of it [4]
* The webconsole filter/clear buttons look really weird when the toolbox is narrow [5]

Everything else is gorgeous though!

Comment 64

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [4]

Comment 65

[Victor Porof [:vporof][:vp]]

Attachment: screenshot [5]

Comment 66

[Brian Grinstead [:bgrins]]

Attachment: console-buttons-now.png

Good catch on the console buttons - I've updated them to not clear out the text on the clear button and to make the really big circles a bit smaller

Comment 67

[Brian Grinstead [:bgrins]]

Attachment: crumb-alignment.png

> * Is it just me, or does it look like the text inside the breadcrumbs
> buttons isn't vertically centered? Compared to the toolbar tabs, it's 1 or 2
> px lower than the baseline in the tabs.

Maybe?  I unscientifically drew some lines and if you look at the 'l' characters maybe it is off by a px or two.

Comment 68

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-part2.patch

Rebased and fixed a couple of issues from Comment 63.  Going to fold the two patches together for landing

Comment 69

[Brian Grinstead [:bgrins]]

Attachment: thin-toolbar-r=vporof.patch

Folded and built on top of Bug 1023666 to update the new reload button in the netmonitor.

Comment 70

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=666b0c51e70c

Comment 71

[Brian Grinstead [:bgrins]]

Going to land this in front of Bug 984051 and add the standalone attributes there

Comment 72

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #71)
> Going to land this in front of Bug 984051 and add the standalone attributes
> there

Never mind, that landed already

Comment 73

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/9fd9c035a76a

Comment 74

[Ed Morley [:emorley]]

Backed out changeset 9fd9c035a76a (bug 942292) for making browser_canvas-frontend-slider-02.js fail 30-40% of the time on opt linux runs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41680601&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=41685001&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=41688596&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=41689162&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=41689109&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=41685415&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/2cb7a1c95803

Comment 75

[Brian Grinstead [:bgrins]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #74)
> Backed out changeset 9fd9c035a76a (bug 942292) for making
> browser_canvas-frontend-slider-02.js fail 30-40% of the time on opt linux
> runs:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41680601&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41685001&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41688596&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41689162&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41689109&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41685415&tree=Fx-Team
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/2cb7a1c95803

I don't understand how this patch would trigger that error (this is CSS/HTML changes only).  Still, I have a try push where I've excluded anything that was touching the canvas debugger files to see if that resolves it: https://tbpl.mozilla.org/?tree=Try&rev=ef80a95ad18d.

I've noticed looking through tbpl that this same error has been coming up quite a bit lately (on different canvas debugger tests), including on the push where this was backed out: https://tbpl.mozilla.org/?tree=Fx-Team&rev=2cb7a1c95803 and the push before this originally landed: https://tbpl.mozilla.org/?tree=Fx-Team&rev=d3d04ca08d9c.

The actual error here is:

07:57:21     INFO -    Message: TypeError: Not enough arguments to CanvasRenderingContext2D.fillRect.
07:57:21     INFO -    Stack:
07:57:21     INFO -      ContextUtils.replayAnimationFrame@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/canvas.js:622:11

Comment 76

[Brian Grinstead [:bgrins]]

Looks like something that landed a bit before this caused Bug 1025118.  Once the tree reopens this should be safe to land

Comment 77

[Ryan VanderMeulen [:RyanVM]]

I re-landed this along with the backout for bug 1025118.
https://hg.mozilla.org/integration/fx-team/rev/a981121ef93b

Comment 78

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a981121ef93b