freddi34 Reporter
	Description • 9 years ago

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.

	[:Aleksej]
	Comment 1 • 9 years ago

bug 1131978?

	Karl Tomlinson (:karlt)
	Comment 2 • 9 years ago

This needs the reverse scaling described in the second to last paragraph of
https://bugzilla.mozilla.org/show_bug.cgi?id=975919#c13

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 3 • 9 years ago

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.

	freddi34 Reporter
	Comment 4 • 9 years ago

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.

	Andrew Comminos [:acomminos]
	Comment 5 • 9 years ago

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.

	Andrew Comminos [:acomminos]
	Comment 6 • 9 years ago

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.

	Andrew Comminos [:acomminos]
	Comment 7 • 9 years ago

Karl, this patch implement the inverse scaling you mentioned on the original GTK3 HiDPI bug, as well as bounds scaling. Thanks in advance.

	Karl Tomlinson (:karlt)
	Comment 8 • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8585242 [details] [diff] [review]
Scale widgets on HiDPI GTK3.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34275ce2b9d4

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 10 • 9 years ago

This one moves gdk_screen_get_monitor_scale_factor() to nsScreenGtk::GtkGetMonitorScaleFactor().

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 11 • 9 years ago

(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.

	Carsten Book [:Tomcat]
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1381d4b4a461

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1381d4b4a461

	Karl Tomlinson (:karlt)
	Comment 14 • 9 years ago

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.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 16 • 9 years ago

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

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 17 • 9 years ago

Try looks good

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/466a8f140def

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/466a8f140def

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 21 • 9 years ago

Comment on attachment 8598619 [details] [diff] [review]
GetWidgetBorder() scale patch

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b084f6111bd7

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 22 • 9 years ago

Try looks good.

	Pulsebot
	Comment 23 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b29c36c4c0

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b29c36c4c0

	Phil Ringnalda (:philor)
	Comment 25 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b3b29c36c4c0