Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KKoukiou opened this issue Jan 5, 2023 · 8 comments
Open

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

KKoukiou opened this issue Jan 5, 2023 · 8 comments
Labels

Comments

@KKoukiou
Copy link
Contributor

KKoukiou commented Jan 5, 2023

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.

patternfly/patternfly-design#1140

@martinpitt
Copy link
Member

martinpitt commented Jan 5, 2023

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.

@martinpitt martinpitt added bug and removed enhancement labels Jan 5, 2023
@martinpitt martinpitt changed the title Follow some more fine grained approach for applying the tabular fonts class from Patternfly Carefully opt into tabular fonts instead of using them everywhere Jan 5, 2023
@garrett
Copy link
Member

garrett commented Jan 9, 2023

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
Copy link
Member

garrett commented Jan 9, 2023

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
Copy link
Member

garrett commented Jan 9, 2023

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

@garrett
Copy link
Member

garrett commented Jan 9, 2023

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. 😞 (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. 😢

@garrett
Copy link
Member

garrett commented Jan 9, 2023

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
Copy link
Member

garrett commented Jan 9, 2023

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

KKoukiou pushed a commit to garrett/cockpit that referenced this issue Jan 10, 2023
Partially addresses cockpit-project#18125

This is the least intrusive change; fonts are embedded as to:
- Not require buildsystem changes to ship font files
- Not inflate number of connections from browser to Cockpit
- Be easily portable to cockpit-machines, cockpit-podman, etc.

It's messy when viewing the stylesheet, so they're at the bottom of the
file.

Additionally, subsetting and mapping these custom versions of the fonts
are much smaller in size than forking the font and using our own custom
version, which would be worse.

Note: PatternFly, and therefore Cockpit, uses non-standard weights so
medium (semi-bold) becomes bold. Additionally, only regular and medium
weights are used, so we're only including these two weights and their
italic variants in the override. (Leaving out light, bold, and heavy
weights and their italic variants).
KKoukiou pushed a commit that referenced this issue Jan 10, 2023
Partially addresses #18125

This is the least intrusive change; fonts are embedded as to:
- Not require buildsystem changes to ship font files
- Not inflate number of connections from browser to Cockpit
- Be easily portable to cockpit-machines, cockpit-podman, etc.

It's messy when viewing the stylesheet, so they're at the bottom of the
file.

Additionally, subsetting and mapping these custom versions of the fonts
are much smaller in size than forking the font and using our own custom
version, which would be worse.

Note: PatternFly, and therefore Cockpit, uses non-standard weights so
medium (semi-bold) becomes bold. Additionally, only regular and medium
weights are used, so we're only including these two weights and their
italic variants in the override. (Leaving out light, bold, and heavy
weights and their italic variants).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants