756890 - Fix an easy set of GCLI/Developer Toolbar display issues

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Comment 1

[Joe Walker [:jwalker]]

Attachment: Upload 1

Paul - we'll need to ask Dão for review of the browser.css changes, but could you give me feedback on those changes and review of the changes to gcli.css please.
Thanks,

Comment 2

[Joe Walker [:jwalker]]

Attachment: Upload 2

Rebase.

Comment 3

[Joe Walker [:jwalker]]

Attachment: Upload 3

I made a simple change to add a block of defaults to prevent lightweight themes from inadvertently messing up the command line.

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 625680 [details] [diff] [review]
Upload 1

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

This looks hot :) The integration with the panel is very well done (seamless).

(tested on Linux)

Visual remarks, feel free to break these comments in follow-up bugs:
- a one pixel black border on top of the toolbar is missing
- the toolbar is not as high as the inspector toolbar. And I think we want them to have the same size. The height of the inspector toolbar is flexible, and the highest elements are the buttons with images. You might want to add a min-height to the toolbars (or to the highest elements). We can do that later.
- shadow missing at the bottom of the input
- only tested on Linux, but focus is not visible (should we use a blue ring? like in the mockup) (we can do that later)
- the dotted underline changes the size of the toolbar when it appears: http://imgur.com/a/ispnS
- the completion hint is lower by one pixel: http://i.imgur.com/cwXeT.png
- we can't tab though the buttons (they don't get the focus) (we can do that alter)
- vertical alignment of the prompt sign (>>) is wrong (6px on top, 3px on the bottom)

Code:

::: browser/base/content/highlighter.css
@@ +138,5 @@
> +.gclitoolbar-input-node,
> +.gclitoolbar-complete-node,
> +.gclitoolbar-prompt {
> +  direction: ltr;
> +}

Why is that in highlighter.css?

::: browser/themes/gnomestripe/browser.css
@@ +2441,5 @@
> +
> +#gcli-output,
> +#gcli-tooltip {
> +  border: 0;
> +  background: transparent;

nit: we usually try to be explicit:
border-width: 0;
background-color: transparent;

@@ +2451,5 @@
>  .gclitoolbar-prompt {
>    font-family: monospace;
> +  font-size: 1.1em;
> +  margin-top: 1px;
> +  margin-bottom: 1px;

These margins feel wrong.

@@ +2452,5 @@
>    font-family: monospace;
> +  font-size: 1.1em;
> +  margin-top: 1px;
> +  margin-bottom: 1px;
> +  -moz-margin-start: 10px;

For alignment with other devtools toolbars (inspector) I don't think we should do that (margin-start).

@@ +2465,1 @@
>  }

I think most of this code should be inherited from or shared with .devtools-searchinput.

::: browser/themes/gnomestripe/devtools/gcli.css
@@ +96,5 @@
> +.gcli-row-out h4,
> +.gcli-row-out h5,
> +.gcli-row-out th,
> +.gcli-row-out strong {
> +  color: hsl(210,30%,95%);

Is it possible to avoid the descendant selectors?
Could these elements inherit from .gcli-row-out?

::: browser/themes/pinstripe/browser.css
@@ +3198,5 @@
> +  -moz-padding-end: 4px;
> +  height: 100%;
> +  line-height: 100%;
> +  border: 1px solid transparent;
> +  border-radius: 2px;

border-radius: @toolbarbuttonCornerRadius@; ?

::: browser/themes/pinstripe/devtools/gcli.css
@@ +46,5 @@
> +  border: 1px solid hsl(210,11%,10%);
> +  box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
> +  background-image: url(background-noise-toolbar.png),
> +                    -moz-linear-gradient(top, hsla(209,18%,18%,0.9), hsl(210,11%,16%));
> +  border-radius: 2px;

@toolbarbuttonCornerRadius@ ?

Comment 5

[Paul Rouget [:paul]]

Grr, splinter review…

> .gclitoolbar-input-node,
> .gclitoolbar-complete-node,
> .gclitoolbar-prompt {
> […]

I think most of this code should be inherited from or shared with .devtools-searchinput.

Comment 6

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #5)
> Grr, splinter review…
> 
> > .gclitoolbar-input-node,
> > .gclitoolbar-complete-node,
> > .gclitoolbar-prompt {
> > […]
> 
> I think most of this code should be inherited from or shared with
> .devtools-searchinput.

I'm a bit reluctant to do this right now because there are enough differences that I think we could have troubles:
* .devtools-searchinput styles a textarea whereas the styling for the GCLI input
  is applied to a description under the textarea
* .gclitoolbar-input-node has 2 popups that go above the area
* .gclitoolbar-input-node is flex=1
* .gclitoolbar-input-node has a prompt that (is planned to) change style depending
  on the input

How about we get the GCLI input right and then visit this again?

Comment 7

[Paul Rouget [:paul]]

(In reply to Joe Walker from comment #6)
> (In reply to Paul Rouget [:paul] from comment #5)
> > Grr, splinter review…
> > 
> > > .gclitoolbar-input-node,
> > > .gclitoolbar-complete-node,
> > > .gclitoolbar-prompt {
> > > […]
> > 
> > I think most of this code should be inherited from or shared with
> > .devtools-searchinput.
> 
> I'm a bit reluctant to do this right now because there are enough
> differences that I think we could have troubles:

I understand. You can just reproduce some part of it (copy the property values).
The positioning, the borders and the shadows should be the same I think.
(not the radius for Mac, and not the background for all).

Comment 8

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #4)
> Comment on attachment 625680 [details] [diff] [review]
> Upload 1
> 
> Review of attachment 625680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks hot :) The integration with the panel is very well done
> (seamless).
> 
> (tested on Linux)
> 
> Visual remarks, feel free to break these comments in follow-up bugs:
> - a one pixel black border on top of the toolbar is missing
> - the toolbar is not as high as the inspector toolbar. And I think we want
> them to have the same size. The height of the inspector toolbar is flexible,
> and the highest elements are the buttons with images. You might want to add
> a min-height to the toolbars (or to the highest elements). We can do that
> later.
> - shadow missing at the bottom of the input
> - only tested on Linux, but focus is not visible (should we use a blue ring?
> like in the mockup) (we can do that later)
> - the dotted underline changes the size of the toolbar when it appears:
> http://imgur.com/a/ispnS
> - the completion hint is lower by one pixel: http://i.imgur.com/cwXeT.png
> - we can't tab though the buttons (they don't get the focus) (we can do that
> alter)
> - vertical alignment of the prompt sign (>>) is wrong (6px on top, 3px on
> the bottom)
> 
> Code:
> 
> ::: browser/base/content/highlighter.css
> @@ +138,5 @@
> > +.gclitoolbar-input-node,
> > +.gclitoolbar-complete-node,
> > +.gclitoolbar-prompt {
> > +  direction: ltr;
> > +}
> 
> Why is that in highlighter.css?

Perhaps the real solution is for highlighter.css to be called devtools.css - which is how I was using it - or maybe I miss the reason why highlighter.css needs separating.

I've this in browser.css for now.

> ::: browser/themes/gnomestripe/browser.css
> @@ +2441,5 @@
> > +
> > +#gcli-output,
> > +#gcli-tooltip {
> > +  border: 0;
> > +  background: transparent;
> 
> nit: we usually try to be explicit:
> border-width: 0;
> background-color: transparent;

Fixed.

> @@ +2451,5 @@
> >  .gclitoolbar-prompt {
> >    font-family: monospace;
> > +  font-size: 1.1em;
> > +  margin-top: 1px;
> > +  margin-bottom: 1px;
> 
> These margins feel wrong.
> 
> @@ +2452,5 @@
> >    font-family: monospace;
> > +  font-size: 1.1em;
> > +  margin-top: 1px;
> > +  margin-bottom: 1px;
> > +  -moz-margin-start: 10px;
> 
> For alignment with other devtools toolbars (inspector) I don't think we
> should do that (margin-start).

OK for both

> ::: browser/themes/gnomestripe/devtools/gcli.css
> @@ +96,5 @@
> > +.gcli-row-out h4,
> > +.gcli-row-out h5,
> > +.gcli-row-out th,
> > +.gcli-row-out strong {
> > +  color: hsl(210,30%,95%);
> 
> Is it possible to avoid the descendant selectors?
> Could these elements inherit from .gcli-row-out?

Not easily - The content here is contributed by commands. I think we're likely to have more performance problems by messing with command output (in JS) than by doing something simple (in CSS, implemented in C++)

> ::: browser/themes/pinstripe/browser.css
> @@ +3198,5 @@
> > +  -moz-padding-end: 4px;
> > +  height: 100%;
> > +  line-height: 100%;
> > +  border: 1px solid transparent;
> > +  border-radius: 2px;
> 
> border-radius: @toolbarbuttonCornerRadius@; ?
> 
> ::: browser/themes/pinstripe/devtools/gcli.css
> @@ +46,5 @@
> > +  border: 1px solid hsl(210,11%,10%);
> > +  box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
> > +  background-image: url(background-noise-toolbar.png),
> > +                    -moz-linear-gradient(top, hsla(209,18%,18%,0.9), hsl(210,11%,16%));
> > +  border-radius: 2px;
> 
> @toolbarbuttonCornerRadius@ ?

OK.

Comment 9

[Joe Walker [:jwalker]]

Attachment: Upload 4

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 626491 [details] [diff] [review]
Upload 4

- hint is not aligned with text: http://i.imgur.com/b3EeD.png
- panel doesn't have the dark background, and its size is wrong: http://i.imgur.com/rsGDK.png
- no shadow: http://i.imgur.com/gcR3v.png - see https://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/devtools/common.css#95
- the margin on top of the prompt (">>") is 6px, on the bottom, it's 5px. You should use a min-height:23px (an odd number) to avoid that I think.


>-#inspector-toolbar {
>+.devtools-toolbar {
>   border-top: 1px solid hsla(210, 8%, 5%, .65);
> }

Please use the id of your toolbar instead (some devtools toolbar don't have a border).

> .gclitoolbar-input-node {
>+  padding-left: 20px;
>   background-color: transparent;
>   -moz-appearance: none;
>-  border: none;
>-  padding: 0 0 0 22px;
>+  border: 1px solid hsl(210,11%,10%);

border-color: hsl(210,11%,10%);

Comment 11

[Joe Walker [:jwalker]]

Attachment: Upload 5

Issues fixed.
Looks fine for me on latest Ubuntu, bug I'm not aware that I've fixed anything that would cause your display problems - does this version still look bad for you?

Comment 12

[Paul Rouget [:paul]]

- hint is still not aligned with text: http://i.imgur.com/bNZjG.png
- you probably need to lower the panel a little more with the shadow (-2px should do it): http://i.imgur.com/Ic0vq.png
- not sure if this is supposed to appear like that: http://i.imgur.com/mvQlP.png
- not sure if this is easily fixable, but the output doesn't have the shadow: http://i.imgur.com/Rhfwd.png - maybe a border would do it (feel free to fix that in a follow up bug)

Comment 13

[Paul Rouget [:paul]]

The way the 3 children of the stack are aligned seems a little fragile.

In browser/themes/gnomestripe/browser.css:
> .gclitoolbar-complete-node {
>   padding-left: 21px;
>   padding-top: 5px;

I don't really understand why we need this. I see it's useful to vertically align the completion text, but I don't understand why we need to use that.

http://i.imgur.com/rDiW6.png <- the dotted lines are the outline of the children. The white border is the stack. How comes the 3 outlines are not totally overlapping? And the offset depends on the font-size.

I'm wondering if we should not wrap these 3 elements in boxes.

Comment 14

[Joe Walker [:jwalker]]

(In reply to Paul Rouget [:paul] from comment #13)
> The way the 3 children of the stack are aligned seems a little fragile.
> 
> In browser/themes/gnomestripe/browser.css:
> > .gclitoolbar-complete-node {
> >   padding-left: 21px;
> >   padding-top: 5px;
> 
> I don't really understand why we need this. I see it's useful to vertically
> align the completion text, but I don't understand why we need to use that.
> 
> http://i.imgur.com/rDiW6.png <- the dotted lines are the outline of the
> children. The white border is the stack. How comes the 3 outlines are not
> totally overlapping? And the offset depends on the font-size.
> 
> I'm wondering if we should not wrap these 3 elements in boxes.

Here's what I've learnt:

A nice trick to sort out vertical centering problems is to set 'height: 50px;' on all 3 elements.

The CSS trick of vertical centering using 'line-height: 100%' looks like it's broken with XUL. But it does work with px values.

If you 'height: 50px; line-height: 50px' on all 3 elements, and remove the 'padding-top: 5px;' from complete-node then everything lines up. BUT ...

As soon as we need a hint with an underline (border-bottom), everything expands for the border.

I've tried using '-moz-box-sizing: border-box;', but this is doomed because the expanding elements are inside the complete-node.

I have a solution that involves changing the elements to hboxes, now the borders don't affect the size of the toolbar. BUT vertical centering is broken/

I'm posting where I am so you can see, and because altering the element types isn't easy.

Comment 15

[Joe Walker [:jwalker]]

Attachment: Upload 6

Some changes as noted in previous comment.

To experiment further, add:

  .gclitoolbar-input-node,
  .gclitoolbar-complete-node,
  .gclitoolbar-prompt {
    height: 50px;
  }

And remove padding-top from .gclitoolbar-complete-node

I think this is a significant improvement on what we have currently and we shouldn't delay for some edge cases.

Comment 16

[Paul Rouget [:paul]]

This is better. The 2 inner boxes are correctly aligned.
The textbox is not though (maybe it needs to be in a box too).

(In reply to Joe Walker from comment #15)
> I think this is a significant improvement on what we have currently and we
> shouldn't delay for some edge cases.

I understand that these one-pixel related problems might not seem important, but they reveal a fragile layout.

Changing the font almost break the design every time.
These are the different problems I run into depending on the font I use:
* alignment problem with the text and the hint;
* underline is too low;
* overlap with the prompt;
* when the underline comes up, the toolbar grows by one pixel.

I think we need to fix this layout, and adding boxes is helping here.
These boxes should have align=center though, not stretch. Then it's gonna be easier to change the borders and keep a correct alignment.

Maybe the prompt should not be inside the stack, but outside, in front of the stack:

<hbox>
  <prompt>
  <stack>
    <hbox><description>…</hbox>
    <hbox><description>…</hbox>
</hbox>

If you think this should be fixed in another bug, I'm ok with that as long as it blocks the "pref-on" bug as well.

Comment 17

[Joe Walker [:jwalker]]

Attachment: Upload 7

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 627635 [details] [diff] [review]
Upload 7

Alinements fixed! Great :)

>+#gcli-output,
>+#gcli-tooltip {
>+  border-width: 0;
>+  background-color: transparent;
>+  margin-bottom: -1px;
> }

-2px, at least on Linux (to hide the inner shadow).

Comment 19

[Joe Walker [:jwalker]]

Dão - I'm going to make the -2px change for Paul do you have anything to add?

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 627635 [details] [diff] [review]
Upload 7

>+html|*#gcli-tooltip-frame,
>+html|*#gcli-output-frame {
>+  overflow-x: hidden;
>+}
>+
>+#gcli-output,
>+#gcli-tooltip {
>+  overflow-x: hidden;
>+}

Combine these two rules.

>           <stack class="gclitoolbar-stack-node" flex="1">
>-            <description class="gclitoolbar-prompt">&#187;</description>
>-            <description class="gclitoolbar-complete-node"/>
>+            <hbox class="gclitoolbar-prompt">&#187;</hbox>
>+            <hbox class="gclitoolbar-complete-node"/>
>             <textbox class="gclitoolbar-input-node" rows="1"/>
>           </stack>

Why this change?

>+  /* Lightweight themes can inadvertently break the display,
>+  so we reset common text attributes */
>+  font-size-adjust: none;
>+  font-stretch: normal;
>+  font-style: normal;
>+  font-variant: normal;
>+  font-weight: 400;
>+  text-align: start;
>+  text-decoration: none;
>+  text-indent: 0;
>+  text-overflow: clip;
>+  text-shadow: none;
>+  text-transform: none;
>+  letter-spacing: normal;
>+  word-spacing: 0;

I don't understand what you're doing here.

Comment 21

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #20)
> Comment on attachment 627635 [details] [diff] [review]
> Upload 7
> 
> >+html|*#gcli-tooltip-frame,
> >+html|*#gcli-output-frame {
> >+  overflow-x: hidden;
> >+}
> >+
> >+#gcli-output,
> >+#gcli-tooltip {
> >+  overflow-x: hidden;
> >+}
> 
> Combine these two rules.

Ok.

> >           <stack class="gclitoolbar-stack-node" flex="1">
> >-            <description class="gclitoolbar-prompt">&#187;</description>
> >-            <description class="gclitoolbar-complete-node"/>
> >+            <hbox class="gclitoolbar-prompt">&#187;</hbox>
> >+            <hbox class="gclitoolbar-complete-node"/>
> >             <textbox class="gclitoolbar-input-node" rows="1"/>
> >           </stack>
> 
> Why this change?

It's part of a solution to the alignment problems that Paul was seeing.
Is there a reason why it's not good?

> >+  /* Lightweight themes can inadvertently break the display,
> >+  so we reset common text attributes */
> >+  font-size-adjust: none;
> >+  font-stretch: normal;
> >+  font-style: normal;
> >+  font-variant: normal;
> >+  font-weight: 400;
> >+  text-align: start;
> >+  text-decoration: none;
> >+  text-indent: 0;
> >+  text-overflow: clip;
> >+  text-shadow: none;
> >+  text-transform: none;
> >+  letter-spacing: normal;
> >+  word-spacing: 0;
> 
> I don't understand what you're doing here.

Lightweight themes sometimes contain styles that make the command-line unreadable. This just prevents that from happening by mistake.

So I've 2 issues to fix - Paul's final comment about margin-bottom and your comment about combining those rules. Is there anything else?

Thanks.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Joe Walker from comment #21)
> > >           <stack class="gclitoolbar-stack-node" flex="1">
> > >-            <description class="gclitoolbar-prompt">&#187;</description>
> > >-            <description class="gclitoolbar-complete-node"/>
> > >+            <hbox class="gclitoolbar-prompt">&#187;</hbox>
> > >+            <hbox class="gclitoolbar-complete-node"/>
> > >             <textbox class="gclitoolbar-input-node" rows="1"/>
> > >           </stack>
> > 
> > Why this change?
> 
> It's part of a solution to the alignment problems that Paul was seeing.

You can adjust the margin and padding of descriptions. It's not clear to me what other alignment problems you could encounter.

> Is there a reason why it's not good?

Text nodes are supposed to be children of labels or descriptions in XUL.

> > >+  /* Lightweight themes can inadvertently break the display,
> > >+  so we reset common text attributes */
> > >+  font-size-adjust: none;
> > >+  font-stretch: normal;
> > >+  font-style: normal;
> > >+  font-variant: normal;
> > >+  font-weight: 400;
> > >+  text-align: start;
> > >+  text-decoration: none;
> > >+  text-indent: 0;
> > >+  text-overflow: clip;
> > >+  text-shadow: none;
> > >+  text-transform: none;
> > >+  letter-spacing: normal;
> > >+  word-spacing: 0;
> > 
> > I don't understand what you're doing here.
> 
> Lightweight themes sometimes contain styles that make the command-line
> unreadable. This just prevents that from happening by mistake.

We set a text-shadow for lightweight themes but none of the other properties you touch. Lightweight themes themselves can't set any styles.

Comment 23

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Joe Walker from comment #21)
> > > >           <stack class="gclitoolbar-stack-node" flex="1">
> > > >-            <description class="gclitoolbar-prompt">&#187;</description>
> > > >-            <description class="gclitoolbar-complete-node"/>
> > > >+            <hbox class="gclitoolbar-prompt">&#187;</hbox>
> > > >+            <hbox class="gclitoolbar-complete-node"/>
> > > >             <textbox class="gclitoolbar-input-node" rows="1"/>
> > > >           </stack>
> > > 
> > > Why this change?
> > 
> > It's part of a solution to the alignment problems that Paul was seeing.
> 
> You can adjust the margin and padding of descriptions. It's not clear to me
> what other alignment problems you could encounter.

-moz-box-align:center doesn't work with them as descriptions.

> > Is there a reason why it's not good?
> 
> Text nodes are supposed to be children of labels or descriptions in XUL.

OK, so I've added a <label> inside the <hbox> for the prompt.

Comment 24

[Joe Walker [:jwalker]]

Attachment: Upload 8

Comment 25

[Joe Walker [:jwalker]]

Dão: Are there any other changes you'd like making to this patch? I'm keen to get it landed soon.
Thanks

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 627909 [details] [diff] [review]
Upload 8

>--- a/browser/devtools/commandline/gcli.css
>+++ b/browser/devtools/commandline/gcli.css

>+.gcli-out-shortcut:before,
> .gcli-help-synopsis:before {
>   content: '\bb';
>+  -moz-padding-end: 2px;
> }

Why is the -moz-padding-end in a content stylesheet?

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> .gclitoolbar-input-node,
> .gclitoolbar-complete-node,
> .gclitoolbar-prompt {
>-  font-family: monospace;
>+  font-family: sans-serif;

Why sans-serif rather than keeping the default UI font?

>--- a/browser/themes/pinstripe/browser.css
>+++ b/browser/themes/pinstripe/browser.css

> .gclitoolbar-input-node,
> .gclitoolbar-complete-node,
> .gclitoolbar-prompt {
>-  font-family: Menlo, Monaco, monospace;
>+  font-family: "Lucida Grande";

Why hardcode Lucida Grande here?

>--- a/browser/themes/pinstripe/devtools/gcli.css
>+++ b/browser/themes/pinstripe/devtools/gcli.css

>+.gcli-body {
>-  font: message-box;
>+  font-family: "Lucida Grande";

Why this change?

The same questions basically apply to winstripe.

Comment 27

[Joe Walker [:jwalker]]

Thanks for the review.

(In reply to Dão Gottwald [:dao] from comment #26)
> Comment on attachment 627909 [details] [diff] [review]
> Upload 8
> 
> >--- a/browser/devtools/commandline/gcli.css
> >+++ b/browser/devtools/commandline/gcli.css
> 
> >+.gcli-out-shortcut:before,
> > .gcli-help-synopsis:before {
> >   content: '\bb';
> >+  -moz-padding-end: 2px;
> > }
> 
> Why is the -moz-padding-end in a content stylesheet?

Because it's not style it's a separator between the command-line and the buttons, but I'm happy to move it.

> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> > .gclitoolbar-input-node,
> > .gclitoolbar-complete-node,
> > .gclitoolbar-prompt {
> >-  font-family: monospace;
> >+  font-family: sans-serif;
> 
> Why sans-serif rather than keeping the default UI font?

Because the default is monospace, which is wrong.

> >--- a/browser/themes/pinstripe/browser.css
> >+++ b/browser/themes/pinstripe/browser.css
> 
> > .gclitoolbar-input-node,
> > .gclitoolbar-complete-node,
> > .gclitoolbar-prompt {
> >-  font-family: Menlo, Monaco, monospace;
> >+  font-family: "Lucida Grande";
> 
> Why hardcode Lucida Grande here?
> 
> >--- a/browser/themes/pinstripe/devtools/gcli.css
> >+++ b/browser/themes/pinstripe/devtools/gcli.css
> 
> >+.gcli-body {
> >-  font: message-box;
> >+  font-family: "Lucida Grande";
> 
> Why this change?
> 
> The same questions basically apply to winstripe.

Because the defaults are either monospace or another font (depending on the element). We want them all to be the same, and match the font on the adjacent buttons.

Is there is a @shortcut@ that we should be using?

If we can confirm today that all I need to do is to move the -moz-padding-end and maybe swap a font-family name, then I'm happy to do that, otherwise I suggest we open a followup bug to fix these issues.

Thanks.

Comment 28

[Dão Gottwald [:dao]]

(In reply to Joe Walker from comment #27)
> > >--- a/browser/themes/gnomestripe/browser.css
> > >+++ b/browser/themes/gnomestripe/browser.css
> > 
> > > .gclitoolbar-input-node,
> > > .gclitoolbar-complete-node,
> > > .gclitoolbar-prompt {
> > >-  font-family: monospace;
> > >+  font-family: sans-serif;
> > 
> > Why sans-serif rather than keeping the default UI font?
> 
> Because the default is monospace, which is wrong.

The default UI font isn't monospace. It's possible that your nodes have a parent where some devtools stylesheet sets a monospace font, but then "font-familily: monospace;" (which you removed above) would have been redundant. So it looks like you should be able to just remove font-family here.

> > >--- a/browser/themes/pinstripe/devtools/gcli.css
> > >+++ b/browser/themes/pinstripe/devtools/gcli.css
> > 
> > >+.gcli-body {
> > >-  font: message-box;
> > >+  font-family: "Lucida Grande";
> > 
> > Why this change?
> > 
> > The same questions basically apply to winstripe.
> 
> Because the defaults are either monospace or another font (depending on the
> element). We want them all to be the same, and match the font on the
> adjacent buttons.
> 
> Is there is a @shortcut@ that we should be using?

message-box gets you the default UI font. In browser.xul or any other document using global.css, you'll inherit that automatically unless you changed it manually on some parent node.

Comment 29

[Joe Walker [:jwalker]]

Attachment: Upload 9

Thanks for the review.

So, this patch:
- moves the moz-padding-end
- removes the font-family from browser.css (the difference in font-family was a
  carry-over from HTML)
- does not remove font-family from gcli.css - it really is needed there,
  and message-box doesn't cut it either

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 628788 [details] [diff] [review]
Upload 9

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> .gclitoolbar-input-node,
> .gclitoolbar-complete-node,
> .gclitoolbar-prompt {
>-  font-family: monospace;
>+  font-size: 1.0em;

"font-size: 1.0em" isn't a no-op here?

>--- a/browser/themes/gnomestripe/devtools/gcli.css
>+++ b/browser/themes/gnomestripe/devtools/gcli.css

>-#gclichrome-body {
>+.gcli-body {
>   margin: 0;
>-  padding: 0;
>-  font: message-box;
>+  font-size: 0.825em;
>+  font-family: message-box;

AFAIK you actually need "font", "font-family: message-box" isn't going to work. This might also make the reduced font-size unnecessary.

>--- a/browser/themes/pinstripe/devtools/gcli.css
>+++ b/browser/themes/pinstripe/devtools/gcli.css

>-#gclichrome-body {
>+.gcli-body {
>   margin: 0;
>-  padding: 0;
>-  font: message-box;
>-  color: #fff;
>+  font-size: 0.825em;
>+  font-family: "Lucida Grande";
>+  color: hsl(210,30%,85%);
>+}

Can you clarify how message-box doesn't cut it here?

Comment 31

[Joe Walker [:jwalker]]

(In reply to Dão Gottwald [:dao] from comment #30)
> Comment on attachment 628788 [details] [diff] [review]
> Upload 9
> 
> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> > .gclitoolbar-input-node,
> > .gclitoolbar-complete-node,
> > .gclitoolbar-prompt {
> >-  font-family: monospace;
> >+  font-size: 1.0em;
> 
> "font-size: 1.0em" isn't a no-op here?

No.

> >--- a/browser/themes/gnomestripe/devtools/gcli.css
> >+++ b/browser/themes/gnomestripe/devtools/gcli.css
> 
> >-#gclichrome-body {
> >+.gcli-body {
> >   margin: 0;
> >-  padding: 0;
> >-  font: message-box;
> >+  font-size: 0.825em;
> >+  font-family: message-box;
> 
> AFAIK you actually need "font", "font-family: message-box" isn't going to
> work. This might also make the reduced font-size unnecessary.

I'll change it back to font.

> >--- a/browser/themes/pinstripe/devtools/gcli.css
> >+++ b/browser/themes/pinstripe/devtools/gcli.css
> 
> >-#gclichrome-body {
> >+.gcli-body {
> >   margin: 0;
> >-  padding: 0;
> >-  font: message-box;
> >-  color: #fff;
> >+  font-size: 0.825em;
> >+  font-family: "Lucida Grande";
> >+  color: hsl(210,30%,85%);
> >+}
> 
> Can you clarify how message-box doesn't cut it here?

The font is wrong. It's serif without this.

Please could you r+ this patch, so we could land it. Thanks.

Comment 32

[Dão Gottwald [:dao]]

(In reply to Joe Walker from comment #31)
> (In reply to Dão Gottwald [:dao] from comment #30)
> > >--- a/browser/themes/gnomestripe/browser.css
> > >+++ b/browser/themes/gnomestripe/browser.css
> > 
> > > .gclitoolbar-input-node,
> > > .gclitoolbar-complete-node,
> > > .gclitoolbar-prompt {
> > >-  font-family: monospace;
> > >+  font-size: 1.0em;
> > 
> > "font-size: 1.0em" isn't a no-op here?
> 
> No.

Where do these nodes get a different font-size from?

> > >-#gclichrome-body {
> > >+.gcli-body {
> > >   margin: 0;
> > >-  padding: 0;
> > >-  font: message-box;
> > >-  color: #fff;
> > >+  font-size: 0.825em;
> > >+  font-family: "Lucida Grande";
> > >+  color: hsl(210,30%,85%);
> > >+}
> > 
> > Can you clarify how message-box doesn't cut it here?
> 
> The font is wrong. It's serif without this.

Maybe you changed it to "font-family: message-box", which is just broken and can therefore leave the text serif. "font: message-box" should work.

Comment 33

[Joe Walker [:jwalker]]

Attachment: Upload 10

Comment 34

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=a24414165cd4

Comment 35

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

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

Comment 36

[Joe Walker [:jwalker]]

Thanks for helping get this landed Dão and Paul.