1535956 - [meta] Change devtools-toolbar height from 24px to 28px height

Status: NEW

(DevTools :: Shared Components, enhancement)

Description

[Florens Verschelde :fvsch]

We have a shared .devtools-toolbar style that defines a 24px-tall toolbar, but Console and Network override this style to use a 28px height, and Debugger comes with its own 28px-tall toolbar.

I proposed in https://github.com/firefox-devtools/ux/issues/52 to change the Inspector toolbar to 28px as well. Since Inspector uses a shared style, I think we should just update .devtools-toolbar.

A few things to consider:

• There are a few places where we may still want to use a thinner, 24px toolbar, especially for footers (Debugger, Network).
• We have some button and text input styles where the button or input as a 1px block margin. With the extra space, I find it looks better to use a 2px block (top/bottom) margin here.

Comment 2

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 1: define 28px and 24px toolbar styles; r=ntim

Comment 3

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 2: use new toolbar heights in Console and Accessibility; r=nchevobbe,yzen

Depends on D25693

Comment 4

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 3: use new toolbar heights in shared components; r=ntim

Depends on D25694

Comment 5

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 4: use new toolbar heights in Netmonitor; r=honza

Depends on D25695

Comment 6

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 5: use new toolbar heights in Memory and Performance; r=gregtatum

Depends on D25696

Comment 7

[Florens Verschelde :fvsch]

Attachment: Bug 1535956 - Part 6: use new toolbar heights in Inspector; r=rcaliman

Depends on D25697

Comment 8

[Florens Verschelde :fvsch]

A quick explanation of why there are so many patches and what they try to do.

Turns out that when you change the .devtools-toolbar height to something a bit bigger:

1. A few things get broken because we used the devtools-toolbar class on things that are not exactly toolbars.
2. A lot of stuff does not look vertically aligned anymore, and need to get fixed. Things get a bit complicated by the HTML-vs-XUL issue.
3. It reveals that some toolbars are align-items: stretch while others are align-items: center, semi-randomly. I settled on stretch with 2px top/bottom margins on items (instead of the previous 1px margins). Only exception: the filter buttons in Console and Netmonitor.

On top of that, I tried to keep other fixes (aka bugfix creep) low, except maybe in the Inspector where I also fixed some spacing and alignment issues with the breadcrumbs, the toolbar-ish "add a class" and "toggle pseudo-classes" parts of Rules, and some tiny spacing issues in Layout.

A cool way to test these patches is to arc patch D25698 and take it for a spin.
For added fun, you could change these variables in variables.css to other values (e.g. 51px and 21px):

  --theme-primary-toolbar-height: 29px;
  --theme-secondary-toolbar-height: 25px;

Now praying that mochitests do not murder me.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d7c9923779c516e07c50394840f4e92fa2ee480

Comment 9

[Greg Tatum [:gregtatum]]

I'm out at a conference, but I'll check out the patch next week. The code looks good on the patch I'm requested for review, but I'd like to try it out manually.

Comment 10

[Florens Verschelde :fvsch]

Updated all patches following reviews.
New Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ff706990def817ad1a25e00d8a256040894631

Comment 11

[Florens Verschelde :fvsch]

I would like to see this design update through, but there are a few challenges:

1. My patches still lack some reviews or fixes.
2. They have a bit of scope creep (making everything in those toolbars look "right").
3. We updated the Inspector and Console toolbars and the filter inputs since then, so a rebase would be gnarly.

So I'm going to close my patches, and I'd like to proceed like this:

• Treat this bug as a meta/parent bug
• File one bug per panel that needs updates to its toolbars
• Land fixes progressively

Since I can't work full-time on this, this will be much more manageable than a big patch list.

Comment 12

[Florens Verschelde :fvsch]

New strategy: keep .devtools-toolbar at 24px, create a taller variant, and start applying it to more panels progressively. This will allow us to land patches progressively.

1. Bug 1512513: define a --theme-toolbar-height: 24px variable, and make sure that .devtools-toolbar uses this height (+1px for the border).

2. Bug 1577536: Create a variable (--theme-toolbar-tall-height: 28px) and a modifier class (.devtools-toolbar-tall) for the taller toolbars + apply this style on Console and Network.

3. Bug 1623137: Apply the taller style to Inspector toolbars.

4. Refactor Debugger styles to use .devtools-toolbar and .devtools-toolbar-tall (or at least the CSS variables, if using the classes proves impractical).

We won't need to migrate all the other panels to the taller style, at least not immediately.