cockpit-project / cockpit

Cockpit is a web-based graphical interface for servers.
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
11.13k stars 1.1k forks source link

Carefully opt into tabular fonts instead of using them everywhere #18125

Open KKoukiou opened 1 year ago

KKoukiou commented 1 year ago

We now insert this class on the top of all page, thus making anything numeric (there are exceptions ?!) use tabular fonts. We should rather focus on adding this only to elements that need this. For example sliders values.

https://github.com/patternfly/patternfly-design/issues/1140

martinpitt commented 1 year ago

Big +1. IMHO this looks super-ugly right now, as the monospaced text is being used overzealously everywhere. We have wild font inconsistencies like this now:

image

image

Notice how we format zeros differently in the Configuration card, and how ugly the dates are.

Monospaced numbers only make sense in some tabular environments where they actually need to be aligned. The only place where this is the case on the overview page is here:

image

But ironically it doesn't even help fully, as the progress bars still have different lengths. It does help to not make the memory bar "jumpy" if the used amount changes a little, though.

I went through all the pages, and the only other potential usages of tabular numbers that I found are on hwinfo:

image

(Although it's not really necessary there)

and to ensure properly aligned timestamps on the Networking page's Log view:

image

But in all of these cases we'd actually be better off with using a proper monospace font.

In other words: These need to be opt-in and carefully/explicitly used, instead of "everywhere". They are called tabular for a reason?

Also, I find these zeros super ugly, at first I thought my system was broken and somehow missed proper fonts -- but that's a matter of taste. But more objectively, paragraph text or most text which doesn't rely on alignment should use proportional fonts.

garrett commented 1 year ago

The 0 with a slash is a bug in the font; it should have variants with and without the slash, and it should be controlled by a font feature (which is exposed in CSS).

Tabular numbers are needed for columns in tables that have numbers and anywhere where numbers are compared, including numbers compared with themselves over time (such as progress bars and sliders) and numbers compared with other numbers (which is why I mentioned columns in tables). It's also useful for things like IP addresses.

Tabular numbers are not needed for a mix of alphanumeric instead of just numeric where it's not being compared or changing over time.

So we really have 3 issues:

  1. Don't apply everywhere.
  2. Apply in circumstances listed above
    • number comparison, numbers that change, numbers with units, IP addresses, etc.
  3. Fix the font so it doesn't use 0 with a slash.
    • Which may require me doing a hotfix to the font with a monkeypatch in the meantime, which we've done before, as the font is quite slow to update and propagate through PatternFly to us. This would be done via font subsetting and adding a font to a typeface for a specific unicode range (in this case: one character).
    • We actually want font-variant-numeric: tabular-nums; not font-variant-numeric: tabular-nums slashed-zero; (which is what we're seeing) https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-numeric#values
garrett commented 1 year ago

But ironically it doesn't even help fully, as the progress bars still have different lengths. It does help to not make the memory bar "jumpy" if the used amount changes a little, though.

No, that's another bug. It's properly solved with subgrid or display: contents, but we've been waiting on all browsers to catch up. (I really mean we've been waiting for years for Chrome, specifically. Firefox had this for years and even Safari has had support for a little while already.)

And this is only a problem with usage bars.

For progress bars, it's actually still an issue elsewhere.

garrett commented 1 year ago

We also need to report this upstream to both PatternFly and RedHatFont.

garrett commented 1 year ago

I've filed upstream @:

garrett commented 1 year ago

RedHatMono also seems to have the slash. I guess this also needs a patch. And I guess this means I'll have to do type "surgery" in FontForge again with a subset for a hotfix. :disappointed: (And for the various variants as well, such as bold and possibly italic if there's a mono form.)

~Hopefully the metrics are the same between RedHatMono and RedHatText and we can just use the same subset for both.~ edit: Metrics are different. :cry:

garrett commented 1 year ago

I've made a CSS patch locally, to remove the slash!

Before:

image

After:

image

As this is working, the plan would be to proceed by modifying all the font variants (various weights & italic versions of each).

garrett commented 1 year ago

Here's an image diff; it shows that the metrics are untouched; the only difference really is the slash. (There's a little bit of hinting shift in the glyph, but it's sub-pixel. The usage bars are slightly different with values.)

image