1520440 - Define DevTools main font-size/line-height combinations in variables.css

Status: NEW

(DevTools :: Framework, enhancement, P3)

Description

[Florens Verschelde :fvsch]

The font size and line-height situation in DevTools has a few inconsistencies and some (lots of?) hardcoded values.

Until late 2018, roughly 80% of the UI was depending on the browser's default font-size, which was generally 11px on Windows or macOS (not sure about Windows accessibility options though), and 11pt aka 14.6666px on Linux (and sometimes bigger if the user had changed their system settings).

We fixed it and introduced two devtools-wide variables:

:root {
  /* Text sizes */
  --theme-body-font-size: 11px;
  --theme-code-font-size: 11px;
}

Following exploration in https://github.com/devtools-html/ux/issues/2 I would like to complete these variables to create a set of 3 text styles:

1. Default text: sans-serif, 11px font-size, 14px line-height
2. Header text: sans-serif, 12px font-size, 16px line-height
3. Code: monospace, 11px font-size, 15px line-height (discussed for the Inspector markup view and Rules in bug 1508036, and for Style Editor and Debugger source code on-and-off)

Ideally, I would like to have 3 variables for each style, including the font-family, but the sans-serif font-family we use is a system one retrieved using code like font: message-box; (we could use font-family: system-ui, sans-serif but would need bug 1226042 to be implemented for that).
We could perhaps move those lines in common.css to variables.css though:
https://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#38-48

The scope of this bug would be to add the relevant variables in variables.css, document them using a CSS comment and use them in a few relevant places so that they are not unused. Modifying all the CSS codebase to use them instead of hardcoded values or line-height: auto (UA style) would be done later and progressively.

The overall goals for specifying a limited number of font-size/line-height pairs in variables.css and using them for most of the UI would be:

1. Consistency between panels and between operating systems and resolutions (especially for line-height, whose auto value can give us between 12.5px and 14px results depending on OS and font, for a 11px font-size).

2. Ability to bump the font-sizes in the future. In 2006, proposing to use 16px text on the Web seemed revolutionary, and is now standard for articles or even considered too small. Screens tend to get smaller pixels, even when accounting for high-density (e.g. 320dpi at 2x still means that a CSS pixel will render much smaller than on a 110dpi screen at 1x). We may need to adapt by bumping text size in 2019 or 2020.

3. Ability to offer a "bigger fonts" option (related to #2), and perhaps make it the default for new users.

It's not 100% sure that we will ever go for #2 and #3, but even just #1 is worth getting started on this, and we can do the more costly work of removing hardcoded font-sizes later on if and when we want to do #2/#3.

Comment 1

[Florens Verschelde :fvsch]

Related bugs that I would probably rebase on this work:

• Bug 1508036
• Bug 1519904

Comment 2

[Florens Verschelde :fvsch]

Attachment: Bug 1520440 - Add theme variables for common font styles; r=pbro

Comment 3

[Florens Verschelde :fvsch]

Pushed a tentative patch to Phabricator.

For now the code line-height is set to 14px, but I'm planning to change it to 15px in one of the bugs I mentioned before.

Comment 4

[Florens Verschelde :fvsch]

Tim, I would love to have your opinion on those changes if you have the time.

Comment 5

[Tim Nguyen :ntim]

I'm honestly not a fan of using variables to do this, because classes would be a nicer way to do this IMHO, in terms of avoiding code duplication (and maybe performance too)... but I agree that the current situation isn't ideal either.

The current patch seems to fix/set the font-size/line-height combo everywhere, I would much prefer if we encouraged the usage/addition of helper classes such as .devtools-monospace or .theme-body (I'm open to changing the class names too) and remove the variables altogether. For inputs/textarea/buttons, we could just use font: inherit or set the correct class (to override). This way we'd be setting the font-size/line-height combo only once on those classes.

What do you think ?

Comment 6

[Tim Nguyen :ntim]

Looking at the patch again, it looks like the body font size/line height is already set on :root, so we probably don't even need a class for the body font, since we can just let CSS inheritance work its way through.

Comment 7

[Tim Nguyen :ntim]

Another advantage of doing using classes, is that we ensure the right font combo is used, because in the current setup, it's well possible that an element has --theme-code-font-size/--theme-code-line-height, but has the non-monospace font-family.

Comment 8

[Florens Verschelde :fvsch]

Thanks Tim. I agree with you points re:

• Variables could be mix-and-matched in ways we don't want, while classes can define related properties together.
• Code duplication: just applying the "header" variables to tabs and accordion headers shows some code duplication, and applying those variables everywhere would create even more.

I would still like to have those design primitives defined as CSS variables so we can:

• Use them in context that may include variables.css but not common.css (about:debugging does that, maybe that's the only one).
• Use them exceptionally when using the classes is impractical.
• And enable pie-in-the-sky features such as user theming.

For user theming, if we ever want to support it, I'm thinking about a WebExtension which would define a devtools_theme manifest in JSON:

{
  "selection_background": "lime",
  "selection_color: "black",
  ...
}

With a bit (well, a lot) of cleanup, variables.css becomes a blueprint for theming.
Of course we may never ever pursue that, but I think the process for getting closer to that still helps the project's code quality (e.g. removing rarely used or redudant variables).

Comparing different approaches

The downside of having variables + utility classes is that we can end up using the variables in the codebase where it would be better to just use the classes.

The downside of having just utility classes is that they may do too much for very specific uses, and that if someone adds a font-weight: 400; or similarly innocuous style to a text utility class (e.g. .devtools-text-code) it can create subtle bugs in places where we don't expect that font-weight. Utility classes are basically written once, changed never (or with great caution).

naming classes

I think changing devtools-monospace would create bugs; it might be simpler to add a similar class which also sets the font-size and line-height, and start replacing devtools-monospace where we can.

Not sure we need a class for the "body" style. We need to specify it on buttons, but maybe font: inherit is enough.

We could try something like:

/**
 * Override wrong system font from forms.css
 * Bug 1458224: buttons use a wrong default font-size on Linux
 */
html|button, html|select, html|input {
-  font: message-box;
-  font-size: var(--theme-body-font-size);
-  line-height: var(--theme-body-line-height);
+  font: inherit;
}

And maybe those classes:

.devtools-header-text { ... }
.devtools-code-text { ... }

Bikeshedding welcome. :)

Comment 9

[Tim Nguyen :ntim]

(In reply to Florens Verschelde [:fvsch] from comment #8)

I would still like to have those design primitives defined as CSS variables so we can:

• Use them in context that may include variables.css but not common.css (about:debugging does that, maybe that's the only one).
• Use them exceptionally when using the classes is impractical.
• And enable pie-in-the-sky features such as user theming.

For user theming, if we ever want to support it, I'm thinking about a WebExtension which would define a devtools_theme manifest in JSON:

{
  "selection_background": "lime",
  "selection_color: "black",
  ...
}

With a bit (well, a lot) of cleanup, variables.css becomes a blueprint for theming.
Of course we may never ever pursue that, but I think the process for getting closer to that still helps the project's code quality (e.g. removing rarely used or redudant variables).

I'm not necessary against variables, but from what I've seen, simply having the variables around can lead to improper use of them: eg. having new tools/components only use the variables and not at all using the classes.

In terms of theming, we can always do this later but I'm also not sure if changing fonts/line-height is something we want to let themes do, but that's a different debate :)

Bottom line is, I'm not against variables (there are genuine cases where we can't set classes like libraries), but I'd like to avoid improper use of them.

Comparing different approaches

The downside of having variables + utility classes is that we can end up using the variables in the codebase where it would be better to just use the classes.

The downside of having just utility classes is that they may do too much for very specific uses, and that if someone adds a font-weight: 400; or similarly innocuous style to a text utility class (e.g. .devtools-text-code) it can create subtle bugs in places where we don't expect that font-weight. Utility classes are basically written once, changed never (or with great caution).

I'm definitively not asking to change very little things to use classes, but rather asking some mixins/combos to be using classes. In this case, it avoids repetition of the font-size/family/line-height, and ensures they're set to the correct combinations.

naming classes

I think changing devtools-monospace would create bugs; it might be simpler to add a similar class which also sets the font-size and line-height, and start replacing devtools-monospace where we can.

I'm not too concerned about .devtools-monospace, since it doesn't seem to do more than just set the font-size/family: https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/themes/common.css#66-69

Adding in line-height shouldn't do much harm.

If it makes you more comfortable refactoring, I'm fine with introducing a new class, but I'd be concerned about ending up with 2 classes if you don't have enough time to finish refactoring :)

Not sure we need a class for the "body" style. We need to specify it on buttons, but maybe font: inherit is enough.

Definitively agree with this. I'd simply make sure we don't unnecessarily set font: inherit; on cases where CSS inheritance already works.

We could try something like:

/**
 * Override wrong system font from forms.css
 * Bug 1458224: buttons use a wrong default font-size on Linux
 */
html|button, html|select, html|input {
-  font: message-box;
-  font-size: var(--theme-body-font-size);
-  line-height: var(--theme-body-line-height);
+  font: inherit;
}

Looks good to me.

And maybe those classes:

.devtools-header-text { ... }
.devtools-code-text { ... }

Sounds fine. I can't find better names to suggest, so go ahead with those.