HeyPuter / terminal

⌨️ Web-based terminal in pure JavaScript
https://puter.com/app/terminal
GNU Affero General Public License v3.0
113 stars 15 forks source link

Default font has issues on Linux, add ability to configure monospace font used #5

Closed paulie-g closed 6 months ago

paulie-g commented 6 months ago

See attached screenshot for the results of your font choice on a Linux system with a great many fonts installed (tested in Chromium and Firefox). This needs configurable monospace fonts to be usable.

Screenshot from 2024-03-12 04-10-46

AtkinsSJ commented 6 months ago

Yeah the current font rule is just:

font-family: courier-new, courier, monospace;

which is a bit lacking. On my machine running Ubuntu, it's picked "Nimbus Mono PS" as the font and that looks reasonable: image

Probably we want some combination of:

paulie-g commented 6 months ago

My view is that 'monospace' should be the default - people already have a monospace font configured in their browser, it would make sense to use it by default.

I got rid of an Adobe fonts package I had installed so that:

fc-match courier-new
NimbusMonoPS-Regular.otf: "Nimbus Mono PS" "Regular"

However, this has not fixed it. Is there some way to check which font Chromium ends up with from the devtools? I suspect it either does its own font selection so it's not identical to fc-match, or has an internal cache of some kind. Incidentally, I have the same problem with xtermjs, so it's clearly a systemwide issue with choosing a 'courier'.

I you're going to make the font configurable, make it configurable for the terminal. Firstly, it's unlikely someone is going to want the same font for a terminal and for the rest of the GUI (I use a retro IBM VGA font for the terminal for example). Secondly, the terminal could at some point be useful as a separate project.

AtkinsSJ commented 6 months ago

Is there some way to check which font Chromium ends up with from the devtools?

In Chrome at least, if you select one of the elements, then the "Computed" tab, there's "Rendered fonts" at the bottom:

image

Firefox has a slightly nicer UI that lets you fiddle with the font too. I didn't realise this was here! image

And yes, the way font matching is done in web browsers is pretty involved. I've spent time looking at the spec for it in the past, oof.

I you're going to make the font configurable, make it configurable for the terminal.

Ah yeah, what I said before wasn't clear. I meant that maybe there could be a setting in Puter for the default monospace font to use in any apps that want one. (Or even more specifically, any apps that use a terminal-like interface.) That's how SerenityOS does it and that works pretty well.

Secondly, the terminal could at some point be useful as a separate project.

Yup! I know @jelveh is keen to make the shell as good as possible in its own right. There are instructions for running the terminal in a browser on its own, but also it's possible to run the shell in your native terminal, though that's in a very early state. We're working on it!

paulie-g commented 6 months ago

I've taken a look and:

  1. I now understand why xterm.js and puter terminal have the same problem, you're using xterm.js
  2. The problem is that, by default, the xtermjs rows are selecting a sans font (Liberation Sans in my case). Manually editing the style (on a row or the entire xterm-rows), setting font-family to monospace, fixes most of the issues (the horizontal spacing is still too big, but at least the glyph positioning isn't wonky anymore).

You really should set monospace by default. Even if specific typeface selection worked, no one wants Courier in their terminal.

AtkinsSJ commented 6 months ago

I'm not sure why the existing rule is picking an odd font for you. I can only assume that you have a font on your system that is claiming to be "Courier" or "Courier New". Otherwise it would already fallback to "monospace" and use your browser's default monospace font.

paulie-g commented 6 months ago

The problem isn't that something is claiming to be Courier (fc-match Courier -> Nimbus Mono PS, which would be fine), it's that both Chromium and FF decide they want a sans for some mysterious reason (and therefore choose the default browser-configured Sans typeface). There is no uncertainty here now that you've shown me how to look up the computed fonts.

There is exactly 0 reason to override the user's browser default monospace typeface setting. If you do (this way), clearly some percentage of users is going to have unpredictable typeface resolution resulting in borked output. Few are going to debug the problem or report it, they will just assume your stuff is broken, especially since monospace works perfectly fine elsewhere for them, eg in code blocks in articles/github/etc. Making the correctness of your output dependent on opaque resolution (which doesn't match Freetype resolution, I'm guessing it comes from Skia) of a single font that can not, for legal reasons, be present on Linux systems, puts you on a hiding to nothing.

If you want to impose your monospace typeface preferences on users for some reason, load a webfont and specify it.

In practical terms, it doesn't matter that a) this isn't how it's supposed to work, and/or b) "I can't reproduce it on my machine". This error is not a result of misconfiguration and it affects n users where n > 0 for sure, and likely n > 1 (bounded by upper bound N of users). There's a fix, it's trivial, and is probably the way it should have been done in the first place.

Fundamentally, I could absolutely fix this locally with a userscript if I needed to. I'm just reporting an edge case to an exotic project that's unlikely to have seen wide testing (because that's what I'd want as a dev), but if you think this isn't a problem, that's obviously your prerogative.

AtkinsSJ commented 6 months ago

The problem isn't that something is claiming to be Courier

I respectfully disagree. I don't know how familiar you are with CSS and how font matching works there but the way it works is straightforward. (It also seems a bit different from fc-match: fc-match asdasdasd returns NotoSans-Regular.ttf: "Noto Sans" "Regular" - browsers instead ignore a missing font and fall back to the next option.)

The CSS rule affecting this text is:

font-family: courier-new, courier, monospace;

That means for each character in the text, the browser first looks for it in courier-new if that font exists and has the character, otherwise in courier, and then otherwise in whatever the default monospace font is. By definition, this should not cause the problems you're seeing: if you don't have some version of courier installed, the CSS is equivalent to font-family: monospace; which you say works. Therefore, when browsers try and look for courier or courier-new on your system, they must be getting that wrong sans font. I don't know why, but I don't see how it's possible otherwise.

I do appreciate you reporting problems! And I don't mean to be argumentative, so I hope I'm not coming across that way. I just want to figure out what's going on here.

And as for whether Courier is a good choice, :man_shrugging:. Making it configurable like you say would be the best option. But it shouldn't be producing this problem regardless.

paulie-g commented 6 months ago

[This is not meant to be rude, I was brought up on LKML and I've already deleted half of the comment to make it sound better and failed, soz.]

I understand how it's supposed to work, yes. It clearly doesn't work that way or this bug report wouldn't exist. That font-family stanza is short-circuiting in a way you don't expect. It is immaterial why that happens as long as it's not a misconfig unique to my system. If indeed Liberation Sans, which is being computed, is somehow doing something "wrong", it's doing this across some percentage P of the vast swathe of Linux systems that have it installed.

My point is that a) there's a bug, and b) there's 0 value in overriding the user-defined browser monospace font in CSS, which is what triggers the bug. It's arguably not a good idea even if it works universally, but when it clearly produces unintended behaviour for some subset of users, it's a positively braindead thing to be doing, and I don't quite understand why someone would choose that hill to die on. Unless you think adding a help popup explaining how CSS ought to work is a substitute for acceptable output.

Whether to fix the obvious bug (which we can't figure out the root cause for, but can avoid triggering) with a trivial one-line diff or continue explaining how you think this should ideally work, in the face of evidence that it sometimes doesn't, is up to you. I reported it and debugged it as far as was reasonable for a project I don't use in an ecosystem I'm not au fait with.

paulie-g commented 6 months ago

In case you decide to debug further for your own reasons, I'm attaching the fontconfig debug log of degoogled Chromium loading https://puter.com/app/terminal with FC_DEBUG=1. FC_DEBUG=2 produces a ~5GB log.

font.log

AtkinsSJ commented 6 months ago

No offence taken. I also hope you didn't find me rude. I recognise this is frustrating for both of us. :sweat_smile:

Thank you for the log, that should be very useful.

KernelDeimos commented 6 months ago

Closing this as is related to xtermjs