Should not need a pres context to get a system font.
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
We have system fonts in CSS pixels, convert them to device pixels, and then back to CSS pixels. That's a bit silly.
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 1•6 years ago
|
||
The only caller wants CSS pixels, no need to go back and forth.
This is the last dependency on the pres context, I think, from the style system
font code.
I'm not quite sure what the gtk stuff was trying to do, really, but it's
converting to CSS pixels to something else, and I just tried (I'm on GTK) with
different screen resolutions and such and zoom factors and fonts look fine.
Why not just make the same straightforward transformation of the code in the GTK Case that you're making for all the other cases? (It seems like that case is there for something related to high DPI displays where we use 1.5, 2, 3, etc. device pixels per CSS pixel.)
(Or does it not look fine if you leave that code?)
|
|
Assignee | |
Comment 3•6 years ago
|
||
(In reply to David Baron :dbaron: 🏴 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #2)
Why not just make the same straightforward transformation of the code in the GTK Case that you're making for all the other cases? (It seems like that case is there for something related to high DPI displays where we use 1.5, 2, 3, etc. device pixels per CSS pixel.)
(Or does it not look fine if you leave that code?)
I could just do that, yeah, I guess I'll just do it since it's less risky. Though it looks fishy to me since we don't account for GetFontScaleDPI() except when the font is in pango-points instead of pixels...
I'd note the GTK code being removed comes from 3 places:
https://hg.mozilla.org/mozilla-central/rev/f842d2ba5600 (bug 975919)
https://hg.mozilla.org/mozilla-central/rev/f969c4ecfd05 (bug 1431337)
https://hg.mozilla.org/mozilla-central/rev/d2696b9ab43f (bug 1439857)
|
|
Assignee | |
Comment 5•6 years ago
|
||
Also I don't know why should the result of this function depend on layout.css.appUnitsPerPx
|
|
Assignee | |
Comment 6•6 years ago
|
||
So right now when you are on HiDPI (like I am) and change layout.css.devPixelsPerPx to 1.0, the system fonts don't scale down. I just tested on Miko's computer and they do scale down on Mac.
So I'm pretty sure that the condition on nsIWidget::DefaultScaleOverride() is wrong, or at least inconsistent with what we do on other platforms.
Now to dig on the GetFontScaleFactor bit.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
So right now when you are on HiDPI (like I am) and change layout.css.devPixelsPerPx to 1.0, the system fonts don't scale down. I just tested on Miko's computer and they do scale down on Mac.
I don't think they're supposed to scale down -- they should match the size they are in other apps on your system. Sounds like Mac is buggy.
|
|
Assignee | |
Comment 8•6 years ago
|
||
(In reply to David Baron :dbaron: 🏴 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #7)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
So right now when you are on HiDPI (like I am) and change layout.css.devPixelsPerPx to 1.0, the system fonts don't scale down. I just tested on Miko's computer and they do scale down on Mac.
I don't think they're supposed to scale down -- they should match the size they are in other apps on your system. Sounds like Mac is buggy.
I think every platform behaves the same except GTK. GTK is also buggy since it only accounts for the primary monitor scale. I don't think that check really belongs there.
It is also really weird that all fonts would scale down except system fonts IMO, but that's probably another matter.
|
|
Assignee | |
Comment 9•6 years ago
|
||
I'll just keep the gtk code as is for now, but I think it's broken.
|
||
Comment 10•6 years ago
|
||
This looks like a nice cleanup on the whole, but one thing bothers me a bit. Everywhere else, the size field of gfxFontStyle is (I think) a device-pixel size; gfx/thebes doesn't generally know much about CSS-to-device scaling, that's the responsibility of layout.
It feels hacky, therefore, to have LookAndFeel::GetFont returning a gfxFontStyle record where the size field is (ab)used to hold a CSS px size.
I see you've added a comment in widget/LookAndFeel.h to indicate this, but I wonder if we can do better. Would it be feasible to make LookAndFeel::GetFont return an nsFont instead of the nsString/gfxFontStyle pair? After all, nsLayoutUtils::ComputeSystemFont wants to use the result to set up an nsFont anyhow -- so can we just do that directly in the widget code, and skip the gfxFontStyle altogether?
|
|
Assignee | |
Comment 11•6 years ago
|
||
Yeah, ok, let's use that bug to track the nsFont change. Thanks Karl for pointing it out!
|
||
Comment 12•6 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/mozilla-inbound/rev/56f4c25656d9 Should not need a pres context to get a system font. r=jfkthame
|
||
Comment 13•6 years ago
|
||
| bugherder | ||
Description
•