Closed Bug 1319838 Opened 7 years ago Closed 5 years ago

[Wayland/HiDPI] Draw hi-res GTK widgets on HiDPI screens

Categories

(Core :: Widget: Gtk, defect, P3)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: cassidy, Assigned: stransky)

References

Details

(Whiteboard: tpi:+)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/602.1 (KHTML, like Gecko) Version/8.0 Safari/602.1 elementary OS/0.4 (Loki) Epiphany/3.18.5

Steps to reproduce:

On a HiDPI display on Ubuntu (i.e. the 15" 4K System76 Oryx Pro) with a scaling factor of 2


Actual results:

all theme assets emulated in web pages (i.e. default text entries, dropdowns, check boxes, etc.) are using lodpi assets instead of hidpi. The Ambiance GTK theme itself properly supports HiDPI 2x scaling.


Expected results:

Instead, the emulated GTK widgets should be rendered using the HiDPI assets when being used on a machine with a scaling factor of 2.
Component: Activity Streams: General → Widget: Gtk
Product: Firefox → Core
Thanks for the report.  With Adwaita 3.18, data:text/html,<input type="text"> renders as expected, but data:text/html,<input type="checkbox"> does not.

-gtk-icon-source: -gtk-scaled(url("assets/checkbox-unchecked.png"), url("assets/checkbox
-unchecked@2.png"));

I wonder whether perhaps the -gtk-scaled implementation is only looking at the device scale and not the transformation matrix.  If so, a well placed cairo_surface_set_device_scale() in nsNativeThemeGTK::DrawWidgetBackground() should fix this.

With Adwaita 3.20, a checkbox looks OK, but a check does not.
Blocks: 975919
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: tpi:+
Jan is working on the Gtk/HiDPI issues right now.
Assignee: nobody → jhorak
(In reply to Karl Tomlinson (:karlt) from comment #1)

> I wonder whether perhaps the -gtk-scaled implementation is only looking at
> the device scale and not the transformation matrix.

I did a big dive into the GTK+ code and this is my understanding:

* The -gtk-scaled implementation is not looking at anything at all. It is only a parser for an array of images which is eventually indexed by the scale factor that is stored (indirectly via GtkStyleCascade) in the GtkStyleContext.

* A GtkStyleContext is born with a scale factor of 1. It gets updated when its widget is realized. At that point, the widget knows its screen and display and the scale factor of the display, and sets the style context’s scale factor to the same value.

* None of the widgets that are created in WidgetStyleCache.cpp for the purpose of harvesting style contexts are ever realized.

So my idea is to realize these widgets as soon as they are created. (This will break when/if the mixed-DPI setup becomes supported by GTK+, but I figure a lot of time will pass before then.)

I might have gotten something wrong but I will try this out as soon as I have my next contiguous three- or four-hour block at my HiDPI machine.
Thanks for having a look.

Realizing a widget uses resources, which are otherwise unused.

Some of the style contexts in WidgetStyleCache do not even have widgets.  The GtkStyleContext is designed so that it is not necessary to have a widget.  I expect there is a way to set the scale factor on a GtkStyleContext without requiring a widget.
Indeed there is. Although I would expect a dozen realized widgets are pretty cheap, in comparison to e.g. a hundred tabs I tend to keep.

But okay, I’ll try the tight budget approach first. No realizing widgets unless absolutely necessary.
Yuri, Thanks for looking at it.

Looks like it's more complicated than we even expected. I think if we want to honor the scale factor it's something we need to pass to GetStyleContext() as the rendered widget can be located on different HiDPI/non-hidip screens.

I also wonder how that affects border/padding/margin and other sizes we get from the widget or if those values should be returned un-scaled and are scaled later in gecko layout by actual scale factor (that's the recent case IMHO).
(In reply to Martin Stránský [:stransky] from comment #6)

> Looks like it's more complicated than we even expected. I think if we want
> to honor the scale factor it's something we need to pass to
> GetStyleContext() as the rendered widget can be located on different
> HiDPI/non-hidip screens.

Is that a thing, right now? Does current GTK+ actually support mixed DPI environments? I have not seen any announcements to that effect in the version changelogs.

> I also wonder how that affects border/padding/margin and other sizes we get
> from the widget or if those values should be returned un-scaled and are
> scaled later in gecko layout by actual scale factor (that's the recent case
> IMHO).

That’s a good thing to look out for, thanks. I would expect any such lengths to be specified in CSS pixels (i.e. units of 1/96 in) though, so scaling by the scale factor will probably do the right thing.
(In reply to Yuri Khan from comment #7)
> (In reply to Martin Stránský [:stransky] from comment #6)
> 
> > Looks like it's more complicated than we even expected. I think if we want
> > to honor the scale factor it's something we need to pass to
> > GetStyleContext() as the rendered widget can be located on different
> > HiDPI/non-hidip screens.
> 
> Is that a thing, right now? Does current GTK+ actually support mixed DPI
> environments? I have not seen any announcements to that effect in the
> version changelogs.

It's live in multi-monitor setup when one monitor is HiDPI and one isn't. Widgets are dynamically scaled when window is moved across the screen boundary.

> > I also wonder how that affects border/padding/margin and other sizes we get
> > from the widget or if those values should be returned un-scaled and are
> > scaled later in gecko layout by actual scale factor (that's the recent case
> > IMHO).
> 
> That’s a good thing to look out for, thanks. I would expect any such lengths
> to be specified in CSS pixels (i.e. units of 1/96 in) though, so scaling by
> the scale factor will probably do the right thing.

We already do that by GetMonitorScaleFactor() at nsNativeThemeGTK.cpp. When scale factor changes we reset the themes so this should be covered.
Looks like gtk_style_context_set_scale() can be used here.
Yuri, are you going to create the patch or shall I do so?
Flags: needinfo?(yurivkhan)
Assignee: jhorak → nobody
(In reply to Martin Stránský [:stransky] from comment #10)
> Yuri, are you going to create the patch or shall I do so?

If you have a good idea how to arrange for all the necessary data flow, please do. I’ll be happy to test.
Flags: needinfo?(yurivkhan)

I did some investigation/debugging here and it looks like only the scale is actually correct. What is wrong is the icons rendered as check/radio indicator. Frame around the check is correct as well as buttons/combo boxes for instance.

Looks like gtk_style_context_set_scale() is only half of the solution. Looks like a combination of missing style scale and malformed -gtk-scaled() icon rendering.

Assignee: nobody → stransky

Call gtk_style_context_set_scale() on styles created by WidgetStyleCache module on Gtk 3.20+
Also modify moz_gtk_widget_paint_* routines to pass the scale info to CreateStyleContext()
from WidgetStyleCache.

When we draw on HiDPI display on Linux we scale the widget. On Wayland/HiDPI displays allow to set cairo_surface_set_device_scale() to draw
hi-res widgets by moz_gtk_widget_paint().

Depends on D28466

The patches here are for Wayland only, I don't have appropriate hardware to text X11 HiDPI. Filed Bug 1546343 for it. It should be easy to extend this patch to X11 systems but has to be tested on real HiDPI display on X11.

Summary: [HiDPI] GTK assets always rendered at lodpi → [Wayland/HiDPI] Draw hi-res GTK widgets on HiDPI screens

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82d825105204
[Linux/HiDPI] Set scale factor on styles created at WidgetStyleCache, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/683bd799a54c
[Linux/Wayland/HiDPI] Use cairo_surface_set_device_scale() on HiDPI displays, r=lsalzman

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: