Closed Bug 1144745 Opened 5 years ago Closed 5 years ago

gtk3 build does not render native widgets at correct scale [high-dpi]

Categories

(Core :: Widget: Gtk, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: freddi34, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.76 Safari/537.36 OPR/28.0.1750.40

Steps to reproduce:

I opened the gtk3 build of Firefox nightly 38.0a1 (2015-03-02):
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/elm-linux64/latest/firefox-38.0a1.en-US.linux-x86_64.tar.bz2

I opened various dialogs and webpages with native widgets (buttons, checkboxes, radio buttons, scrollbars).


Actual results:

The widgets are rendered as gtk3 widgets with the gtk3 theme, but not at the correct scale.


Expected results:

The widgets' visual appearance (theme) should have been rendered at twice the width/height.
bug 1131978?
Blocks: gtk3
QA Whiteboard: [bugday-20150323]
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
This needs the reverse scaling described in the second to last paragraph of
https://bugzilla.mozilla.org/show_bug.cgi?id=975919#c13
Status: UNCONFIRMED → NEW
Ever confirmed: true
Unfortunately I don't have any hiDPI system available to test it. The emulation by GDK_SCALE=X is just a debugging workaround which seems to work as expected.
Yes, if the screen has enough pixels, Ubuntu (for example) lets you drag the slider for org.gnome.desktop.interface.scaling-factor to 2. This is basically all what makes the software side hidpi.

Just to be sure: The widgets would ideally not be scaled up in the sense of upsampling (blurry), but the theme would have to know to render them at a larger scale factor so that they appear sharp.
It looks like the majority of the issues here stem from nsNativeThemeGTK::GetMinimumWidgetSize reporting bounds in GTK coordinates; multiplying the final GTK-calculated bounds by the scale factor appears to fix this issue for most widgets. This is similar to the way the Cocoa widget backend does widget scaling.

For the widgets that aren't scaled properly by this change, this appears to be due to unscaled layout calculations (e.g bounds, padding) being done within the paint functions for some widgets in gtk3drawing.c (e.g. moz_gtk_toggle_paint drawing with GTK checkbox metrics and ignoring the passed GdkRectangle's dimensions). I think that the best way to resolve this would be to move all bounds calculation out of the paint functions and into the moz_gtk_{widget}_get_metrics functions so they can be scaled properly by GetMinimumWidgetSize and other bounds-related functions.

Thoughts? I'm working on a patch using this technique at the moment.
Actually, if we're reporting widget sizes in device pixels, when it's time to draw we can just unscale them to GDK coords, apply the GDK scale to the thebes context, and paint widgets normally. That way we get proper layout calculation in device pixels, and don't have to touch gtk3drawing.c at all.
Karl, this patch implement the inverse scaling you mentioned on the original GTK3 HiDPI bug, as well as bounds scaling. Thanks in advance.
Attachment #8585242 - Flags: review?(karlt)
Comment on attachment 8585242 [details] [diff] [review]
Scale widgets on HiDPI GTK3.

(In reply to Andrew Comminos from comment #6)
> Actually, if we're reporting widget sizes in device pixels, when it's time
> to draw we can just unscale them to GDK coords, apply the GDK scale to the
> thebes context, and paint widgets normally. That way we get proper layout
> calculation in device pixels, and don't have to touch gtk3drawing.c at all.

Yes, that's the approach to use.
r+, thank you.

The comments below are regarding remaining issues that can be handled
separately.

GetWidgetBorder() should have similar scaling as this affects the size of
"container" widgets, which are parents of other elements.

>+gint
>+nsNativeThemeGTK::GdkScaleFactor()
>+{
>+#if (MOZ_WIDGET_GTK >= 3)
>+  // Since GDK 3.10
>+  static auto sGdkScreenGetMonitorScaleFactorPtr = (gint (*)(GdkScreen*, gint))
>+      dlsym(RTLD_DEFAULT, "gdk_screen_get_monitor_scale_factor");
>+  if (sGdkScreenGetMonitorScaleFactorPtr) {
>+      // FIXME: In the future, we'll want to fix this for GTK on Wayland which
>+      // supports a variable scale factor per display.
>+      GdkScreen *screen = gdk_screen_get_default();
>+      return sGdkScreenGetMonitorScaleFactorPtr(screen, 0);
>+  }
>+#endif
>+    return 1;
>+}

This code is now in three different places but should really be shared.
Perhaps it could be a static method on nsScreenGtk.
Attachment #8585242 - Flags: review?(karlt) → review+
This one moves gdk_screen_get_monitor_scale_factor() to nsScreenGtk::GtkGetMonitorScaleFactor().
Attachment #8591006 - Flags: review?(karlt)
(In reply to Martin Stránský from comment #9)
> Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34275ce2b9d4

Looks good, asking for check-in.
Comment on attachment 8591006 [details] [diff] [review]
gdk_screen_get_monitor_scale_factor patch

>+  static int32_t GtkGetMonitorScaleFactor();

Thanks.  r+ if you call this GetGtkMonitorScaleFactor().
I'd like to have the Gtk prefix only for GTK identifiers.

The other comments here are optional changes:

I would make the return type gint, to clarify that this is a GTK number.
Usually it is used with other gints from GTK, though not always in practice as Gecko sometimes converts to its types early.

>+        aResult->top *= nsScreenGtk::GtkGetMonitorScaleFactor();
>+        aResult->right *= nsScreenGtk::GtkGetMonitorScaleFactor();
>+        aResult->bottom *= nsScreenGtk::GtkGetMonitorScaleFactor();
>+        aResult->left *= nsScreenGtk::GtkGetMonitorScaleFactor();

Using one GtkGetMonitorScaleFactor() call and keeping the result in a local variable would be a little tidier and more efficient.
Attachment #8591006 - Flags: review?(karlt) → review+
Comment on attachment 8596772 [details] [diff] [review]
gdk_screen_get_monitor_scale_factor v.2 patch

Thanks. There's the a try run for the updated one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=07aed3c653dd
Try looks good
Keywords: checkin-needed
Attachment #8596772 - Flags: checkin?
Attachment #8596772 - Flags: checkin? → checkin+
Blocks: 967100
Attachment #8598619 - Flags: review?(karlt)
Attachment #8598619 - Flags: review?(karlt) → review+
Try looks good.
https://hg.mozilla.org/mozilla-central/rev/b3b29c36c4c0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.