Open Bug 1353596 Opened 7 years ago Updated 2 years ago

Simplify CSS usage of LookAndFeel::GetFont

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

People

(Reporter: xidorn, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1353596 part 1 - Move the Windows-specific size fix for button/field/list into LookAndFeel.
59 bytes, text/x-review-board-request
bzbarsky
: review-
Details
Bug 1353596 part 2 - Make GTK's GetFontImpl use aDevPixPerCSSPixel param.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1353596 part 3 - Make LookAndFeel::GetFont return an nsFont.
59 bytes, text/x-review-board-request
Details 
Simplify some code for bug 1353288 around LookAndFeel::GetFont.
Assignee: nobody → xidorn+moz
The a11y failure in the push above should be fixed by https://hg.mozilla.org/try/rev/14340cffd64f8ae48157ecce1755cb52137ed1a1 but I have no idea about the failure on M(4)... which I cannot reproduce locally...
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #3)
> Created attachment 8854704 [details]
> Bug 1353596 part 3 - Remove aDevPixPerCSSPixel param from
> LookAndFeel::GetFont.
> 
> This parameter was introduced in bug 674373 for HiDPI mode on OS X.
> Nowadays, all impls except Gonk and GTK simply just multiple this
> parameter with the size. Gonk has mostly gone, and GTK impl is fixed
> by the previous patch. So this parameter is no longer useful.

I'm not sure I am entirely happy with this... the issue I see is that LookAndFeel::GetFont returns a gfxFontStyle outparam, but the size field in gfxFontStyle is normally (device) pixels, not CSS px. (See the nsFontMetrics constructor, which applies the AppUnitsPerDevPixel() factor from the device context when setting up a gfxFontStyle to create its gfxFontGroup.)

This patch will change the LookAndFeel::GetFont API such that it uses a gfxFontStyle where the size field is actually CSS pixels instead of device pixels, which seems wrong to me (and if we had more strongly-typed fields everywhere, it wouldn't even compile!).

Perhaps it would be better to change the LookAndFeel::GetFont outparam to an nsFont, so that device-pixel units don't come into the picture there at all (instead of doing that conversion in nsRuleNode::SetFont)?
Attachment #8854702 - Flags: review?(bzbarsky)
Attachment #8854704 - Flags: review?(jfkthame)
Comment on attachment 8854703 [details]
Bug 1353596 part 2 - Make GTK's GetFontImpl use aDevPixPerCSSPixel param.

https://reviewboard.mozilla.org/r/126666/#review129778

::: widget/gtk/nsLookAndFeel.cpp:934
(Diff revision 1)
> -    // Scale fonts up on HiDPI displays.
> +    // |size| is now CSS pixels
> -    // This would be done automatically with cairo, but we manually manage
> -    // the display scale for platform consistency.
> -    size *= ScreenHelperGTK::GetGTKMonitorScaleFactor();
> -
> -    // |size| is now pixels

This is not correct.  Pango pixels are not CSS pixels.  Pango pixels are GTK pixels, which scale to device pixels with GTK's monitor scale factor.

Often that GTK scale factor is 1 and yet CSS pixels are still different from GTK/device pixels due to the system DPI not being 96 or due to layout.css.devPixelsPerPx being set.

System font sizes in device pixels are independent from Gecko parameters.  The returned sizes are always the same number of device pixels as system fonts in other apps (on the same monitor).

(Page zoom changes the size of system fonts when measured in device pixels, which is arguably a bug.  Page zoom does not change the size of native widgets.)
Attachment #8854703 - Flags: review?(karlt) → review-
I suppose I should probably use ScreenHelperGTK::GetSystemDefaultScale() instead? But that looks wrong to me as well.

Per discussion on IRC,
> GTK pixel * ScreenHelperGTK::GetGTKMonitorScaleFactor() = dev pixel
and according to nsDeviceContext::SetDPI, for 100% zoom, [1]
> CSS pixel / dev pixel = 96 / dpi
It means
> CSS pixel = GTK pixel * ScreenHelperGTK::GetGTKMonitorScaleFactor() * 96 / dpi
However, ScreenHelperGTK::GetSystemDefaultScale() is [2]
> ScreenHelperGTK::GetGTKMonitorScaleFactor() * gfxPlatformGtk::GetDPIScale()
and gfxPlatformGtk::GetDPIScale() is [3]
> dpi / 96

Is there something wrong here? Or am I missing something?


[1] https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/gfx/src/nsDeviceContext.cpp#255-256
[2] https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/widget/gtk/ScreenHelperGTK.cpp#145-159
[3] https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/gfx/thebes/gfxPlatformGtk.cpp#370
Flags: needinfo?(karlt)
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #8)
> I suppose I should probably use ScreenHelperGTK::GetSystemDefaultScale()
> instead? But that looks wrong to me as well.

I'm not clear what you are trying to do, but GetSystemDefaultScale() could be
used to convert from device pixels to CSS pixels.

Native theming code returns metrics in device pixels, and native theming does
not know about CSS pixels, and so it would be odd for some LookAndFeel metrics
to be in CSS pixels.

> Per discussion on IRC,
> > GTK pixel * ScreenHelperGTK::GetGTKMonitorScaleFactor() = dev pixel

Yes, but to be clear this is about measurements in pixels, not the size of
pixels.

> and according to nsDeviceContext::SetDPI, for 100% zoom, [1]
> > CSS pixel / dev pixel = 96 / dpi

This DPI is a different DPI to the one returned from
gfxPlatformGtk::GetDPIScale().

gfxPlatformGtk returns the GTK dpi value.

I assume nsDeviceContext uses something similar to
96 * nsIScreen::defaultCSSScaleFactor for that dpi value.

defaultCSSScaleFactor is similar to GetSystemDefaultScale(), but it
is for the particular monitor.  (GTK on X11 happens to always have the
same scale factors on all monitors.  Wayland will be different.)

> It means
> > CSS pixel = GTK pixel * ScreenHelperGTK::GetGTKMonitorScaleFactor() * 96 / dpi

  CSS pixel count = GTK pixel * ScreenHelperGTK::GetGTKMonitorScaleFactor() /
                         nsIScreen::defaultCSSScaleFactor;

> However, ScreenHelperGTK::GetSystemDefaultScale() is [2]
> > ScreenHelperGTK::GetGTKMonitorScaleFactor() * gfxPlatformGtk::GetDPIScale()
> and gfxPlatformGtk::GetDPIScale() is [3]
> > dpi / 96

When layout.css.devPixelsPerPx is not set,

  CSS pixel count = GTK pixel count * gfxPlatformGtk::GetDPIScale()

(but don't assume that layout.css.devPixelsPerPx is not set.)
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #9)
> When layout.css.devPixelsPerPx is not set,
> 
>   CSS pixel count = GTK pixel count * gfxPlatformGtk::GetDPIScale()
> 
> (but don't assume that layout.css.devPixelsPerPx is not set.)

Sorry, that's

  CSS pixel count = GTK pixel count / gfxPlatformGtk::GetDPIScale()
Attachment #8854704 - Attachment is obsolete: true
Comment on attachment 8854702 [details]
Bug 1353596 part 1 - Move the Windows-specific size fix for button/field/list into LookAndFeel.

https://reviewboard.mozilla.org/r/126664/#review130414

::: commit-message-b9610:3
(Diff revision 2)
> +Bug 1353596 part 1 - Move the Windows-specific size fix for button/field/list into LookAndFeel. r?bz
> +
> +Although there is still no way to set those size from system, but it

I don't quite follow what this sentence is saying.   The "Although" clause seems unrelated to the rest of the sentence...

::: commit-message-b9610:7
(Diff revision 2)
> +
> +Although there is still no way to set those size from system, but it
> +keeps the font-size (10pt) for current default settings, which matches
> +Edge and Chrome on Windows.
> +
> +This brings Windows to have the same behavior as other platforms, and

So here's the problem.  From an accessibility point of view, this is horrible.  Users with poor eyesight now literally have no UI to make the text on their button bigger.

The difference is that on other platforms presumably you actually have UI to change the system font for buttons.  So users can do that to make _all_ their buttons have bigger text, in all apps including Firefox.  But on Windows that's not an option, right?

I think that's why we had this code, which at least allowed the default font size the user sets to affect these controls....

::: widget/windows/nsLookAndFeel.cpp:635
(Diff revision 2)
> +    // all pre-determined and cannot be changed by either the control panel
> +    // or programmatically.
> +    // Fields (text fields)
> +    // Button and Selects (listboxes/comboboxes)
> +    // We use whatever font is defined by the system, but explicitly use
> +    // 10 points (13.33px) for consistency with our existing default behavior

The problem is the non-default case.
Attachment #8854702 - Flags: review?(bzbarsky) → review-
Comment on attachment 8854703 [details]
Bug 1353596 part 2 - Make GTK's GetFontImpl use aDevPixPerCSSPixel param.

https://reviewboard.mozilla.org/r/126666/#review130682
Attachment #8854703 - Flags: review?(karlt) → review+
Attachment #8855755 - Flags: review?(jfkthame)
Although we are no longer going to do parse-time system font resolution, some patches here may still be worth landing. But it isn't a priority now, so probably just leave it as-is, and maybe I would revisit at some point...
Heads up: I'm going to remove GetSystemDefaultScale(). (See bug 1356023)
Not actively working on this. Unassign myself.
Assignee: xidorn+moz → nobody
Priority: -- → P3
See Also: → 1526294
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: