1235941 - Firefox doesn't react to GDK scale factor changes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Marco Castelluccio [:marco]]

Firefox doesn't react to DPI changes. If you open it when DPI hasn't been changed yet (as soon as the system starts), you end up with very small icons and text.
Other GTK applications instead (e.g. the GNOME terminal) correctly increase the size of their UI elements.

Comment 1

[Makoto Kato [:m_kato]]

We should use monitors-changed signal.  Patch is coming.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1235941 - Detect DPI change for GTK3. r?karlt

Use monitors-changed signal to detect DPI change.  Also, we should update
mBounds for current scaling setting.

Review commit: https://reviewboard.mozilla.org/r/34725/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34725/

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8718739 [details]
MozReview Request: Bug 1235941 - Detect DPI change for GTK3. r?karlt

https://reviewboard.mozilla.org/r/34725/#review31517

::: widget/gtk/nsWindow.cpp:3375
(Diff revision 1)
> +  // Update mBounds to current scaling setting
> +  Resize(mBounds.x, mBounds.y, mBounds.width, mBounds.height, true);

I doubt requesting a resize from the window manager is the right thing to do here.  GDK should be managing that for us.
What change in mBounds are you expecting here?
GetDesktopToDeviceScale() returns 1.0 and so Resize() won't change mBounds.

::: widget/gtk/nsWindow.cpp:3845
(Diff revision 1)
> +        g_signal_connect(gtk_widget_get_screen(mShell),
> +                         "monitors-changed",

"The ::monitors-changed signal is emitted when the number, size or position of
the monitors attached to the screen change."  That is the signal that
_gdk_x11_screen_set_window_scale emits, but it is also emitted in other
situations.  I think notify::scale-factor on GtkWidget is more appropriate.
configure-event on GtkWidget may be another option, but that's assuming a
little about the implementation.

GTK also has another control affecting scaling, changes in which are signalled
through both notify::resolution on GdkScreen and notify::gtk-xft-dpi on
GtkSettings.  Either of those should be fine I expect, but it's also fine to
leave this for now if you like.

Comment 4

[Makoto Kato [:m_kato]]

https://reviewboard.mozilla.org/r/34725/#review31517

> I doubt requesting a resize from the window manager is the right thing to do here.  GDK should be managing that for us.
> What change in mBounds are you expecting here?
> GetDesktopToDeviceScale() returns 1.0 and so Resize() won't change mBounds.

When dpi is changed, although configure_event is fired, size-allocate isn't fired.  I should call OnSizeAllocate using GtkAllocation after calling  gtk_widget_get_allocation.

> "The ::monitors-changed signal is emitted when the number, size or position of
> the monitors attached to the screen change."  That is the signal that
> _gdk_x11_screen_set_window_scale emits, but it is also emitted in other
> situations.  I think notify::scale-factor on GtkWidget is more appropriate.
> configure-event on GtkWidget may be another option, but that's assuming a
> little about the implementation.
> 
> GTK also has another control affecting scaling, changes in which are signalled
> through both notify::resolution on GdkScreen and notify::gtk-xft-dpi on
> GtkSettings.  Either of those should be fine I expect, but it's also fine to
> leave this for now if you like.

GtkWebKit seems to use "notify::scale-factor".  So I think that "notify::scale-factor" is better instead.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8718739 [details]
MozReview Request: Bug 1235941 - Detect DPI change for GTK3. r?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34725/diff/1-2/

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8718739 [details]
MozReview Request: Bug 1235941 - Detect DPI change for GTK3. r?karlt

https://reviewboard.mozilla.org/r/34725/#review32431

::: widget/gtk/nsWindow.cpp:6005
(Diff revision 2)
> +scale_changed_cb (GtkWidget *widget)

No space before '(' and position change for '*' "GtkWidget* " is Gecko style for new code.

Please specify the GParamSpec* and gpointer parameters explicitly, instead of depending on platform calling convention to drop the trailing unused parameters.  I don't mind whether you include the pspec and user_data names or not.

::: widget/gtk/nsWindow.cpp:6013
(Diff revision 2)
> +    // configure_event is already fired before scaling_facotr signal,

scaling_facotr -> scale-factor

Thanks for the analysis.

Comment 7

[Karl Tomlinson (:karlt)]

Marco, this may not resolve your issue.  If not please feel free to dupe for adding a handler for notify::resolution on GdkScreen or notify::gtk-xft-dpi on GtkSettings.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f6325a50c3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38f6325a50c3