LibWeb: Make default monospace font size 13px #2020
Conversation
This is the default font size used in Chrome and Firefox and my theory is some WPT tests make the same assumption. As an example, https://wpt.live/css/css-text/tab-size/tab-size-spacing-001.html will pass once PR LadybirdBrowser#1999 is merged if we explicitly set the font-size to be 13px. I figured this out when comparing the size of the space character across Chrome and Ladybird, which we were sizing to large and hence overshooting the red square in the aforementioned WPT test. I'd like to think that this is true across all WPT tests that measure pixels and use monospace as their font-family :^) but this is unsubstantiated.
| @@ -1986,6 +1986,7 @@ RefPtr<Gfx::FontCascadeList const> StyleComputer::compute_font_for_style_values( | |||
| case Keyword::Monospace: | |||
| case Keyword::UiMonospace: | |||
| generic_font = Platform::GenericFont::Monospace; | |||
| font_size_in_pt = 13 * 0.75f; | |||
There was a problem hiding this comment.
So if we hit find_generic_font you override the calculated font size? That doesn't seem like the right solution; shouldn't you change the default font size of 16 at line 1814?
There was a problem hiding this comment.
if I change that from 16 to 13 it'll change it for all fonts but we only want to change it for monospaced fonts (i.e font-family: monospace)
There was a problem hiding this comment.
could do something like this so we don't override it until the very end
auto found_font = StyleProperties::font_fallback(monospace, bold);
font_size_in_pt = monospace ? 13 * 0.75f : font_size_in_pt;
font_list->set_last_resort_font(found_font->with_size(font_size_in_pt));There was a problem hiding this comment.
or yeah would need a way to modify CSSPixels font_size_in_px = 16; for only monospaced font's but we don't know that at that point, we would only want to set that is 13 for the generic monospaced font if of course no font-size value has been provided.
There was a problem hiding this comment.
It might help if you can find if this is something that is in a spec somewhere, or if it's custom behavior by both Chrome and Firefox (user agent stylesheet?), etc. Just changing something because WPT expects it is not always the right course of action; e.g. maybe their assumption is wrong and we need to report an issue?
There was a problem hiding this comment.
it's a fair point, I did search the default css in the spec and there is no mention of it, nothing online either. I can only see that this is the behaviour on Firefox and Chrome (if you go into appearance settings it's set there). That's what the FIXME on the line you speak of is working towards I believe (letting the user specify).
Can you please point me to where I'd open an issue or investigate this? In the meantime I can have a search around the WPT tests to see where they specify font-family: monospace and see if any rely on calculations based off of a default value of 13px for this font, which like you've said is probably an assumption to accomodate current existing behaviour that actually isn't in any spec.
There was a problem hiding this comment.
Right, the FIXME is to support user font sizes, and WPT should probably never rely on those.
There was a problem hiding this comment.
Yeah I guess these by default if the user hasn’t specified anything (for the generic fonts) is 13px for monospaced and 16px for everything else (for Chrome and Firefox) which yeah isn’t ideal for WPT to be relying on if that is the case.
Probably better to explicitly set the size in the tests if using monospace or something like that.
I think I may poke around and see if other tests are making this assumption if I can deduce that, and perhaps open or discuss the issue on WPT. Is this the usual process for these types of things?
There was a problem hiding this comment.
I've opened an issue web-platform-tests/wpt#48840 will close this for now and see what comes out of that.
This is the default font size used in Chrome and Firefox and my theory is some WPT tests make the same assumption. As an example, https://wpt.live/css/css-text/tab-size/tab-size-spacing-001.html will pass once PR #1999 is merged if we explicitly set the
font-sizeto be13px.I figured this out when comparing the size of the space character across Chrome and Ladybird, which we were sizing to large and hence overshooting the
red square in the aforementioned WPT test. I'd like to think that this is true across all WPT tests that measure pixels and use monospace as their font-family :^) but this is unsubstantiated.