Closed Bug 1431337 Opened 7 years ago Closed 7 years ago

[hidpi] [wayland] moving window between hidpi and normal dpi monitor does not scale content correctly

Categories

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

Product:
Core
Component:
Widget: Gtk
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jhorak, Assigned: jhorak, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1431337 - Scale content for the actual monitor, not the first one;
(deleted), text/x-review-board-request
stransky
: review+
Details
chevron-layout.css.devPixelsPerPx-2-issue.png
(deleted), image/png
Details
Win 10 with devPixelsPerPx-2.png
(deleted), image/png
Details
Bug 1431337 - Skip hidpi test of startup images because they overflow in Linux
(deleted), text/x-review-board-request
stransky
: review+
Details
When moving window from hidpi monitor to the normal dpi monitor the fonts and ui stays big. Only the page content is resized. The dynamic scaling is currently supported by upstream only in Wayland.
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review219636
Attachment #8943565 - Flags: review?(stransky) → review+
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219638 ::: widget/gtk/nsNativeThemeGTK.cpp:69 (Diff revision 1) > +static inline double > +GetThemeDpiScaleFactor(nsIFrame* aFrame) > +{ > + nsIWidget* rootWidget = aFrame->PresContext()->GetRootWidget(); > + if (rootWidget) { > + return rootWidget->GetDefaultScale().scale; We need to do this correcly. return rootWidget->GetDefaultScale().scale uses GetDefaultScale() which returns layout.css.devPixelsPerPx or GetDefaultScaleInternal() which has different implementation on windows/gtk. GetDefaultScaleInternal() on Windows: monitorDPI/96. GetDefaultScaleInternal() on Linux (roughly): gdk_window_get_scale_factor(actual_screen) * gdk_screen_get_resolution(screen)/96 where screen is the default one (at gfxPlatformGtk). That means this patch depends on which screen/monitor is the default one and messes it up when low/res monitors are used together. Also mixing gdk_window_get_scale_factor() and gdk_screen_get_resolution() is not correct. Please implement nsWindow::GetDefaultScaleInternal() similarry to Windows where monitor(screen) DPI / 96 is returned. ::: widget/gtk/nsNativeThemeGTK.cpp:1586 (Diff revision 1) > // else the minimum size is missing consideration of container > // descendants; the value returned here will not be helpful, but the > // box model may consider border and padding with child minimum sizes. > > nsIntMargin border; > nsNativeThemeGTK::GetWidgetBorder(aFrame->PresContext()->DeviceContext(), This is already scaled so don't apply the scale again.
Attachment #8943566 - Flags: review?(stransky) → review-
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219944 ::: widget/gtk/nsNativeThemeGTK.cpp:69 (Diff revision 1) > +static inline double > +GetThemeDpiScaleFactor(nsIFrame* aFrame) > +{ > + nsIWidget* rootWidget = aFrame->PresContext()->GetRootWidget(); > + if (rootWidget) { > + return rootWidget->GetDefaultScale().scale; I've digged into it more to find out why the current patch works fine and figured out that this approach is actually correct. The nsWindow::GetDefaultScaleInternal [1] computes the scale as monitor scale factor * font scale factor. The font scale factor is computed [2] from FontScaleDPI which is computed by gdk_screen_get_resolution() - in the description of the func is: *Gets the resolution for font handling on the screen*. This resolution is computed from user settings *Scaling factor*. So FontScaleDPI does not depend on the actual monitor dpi but on gsettings *org.gnome.desktop.interface text-scaling-factor*. By default this value is 96, so font scaling is 1, it only jumps to 1.5 when the gtk settings of the scaling factor is more than 1.375. I think we still want to scale widgets by 1.5x when the font scale factor > 1.375 because if user wants to have bigger fonts, they probably want to have bigger widgets. [1] https://searchfox.org/mozilla-central/source/widget/gtk/nsWindow.cpp#825 [2] https://searchfox.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#331 [3] https://developer.gnome.org/gdk3/stable/GdkScreen.html#gdk-screen-get-resolution
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219944 > I've digged into it more to find out why the current patch works fine and figured out that this approach is actually correct. The nsWindow::GetDefaultScaleInternal [1] computes the scale as monitor scale factor * font scale factor. > > The font scale factor is computed [2] from > FontScaleDPI which is computed by gdk_screen_get_resolution() - in the description of the func is: *Gets the resolution for font handling on the screen*. This resolution is computed from user settings *Scaling factor*. > > So FontScaleDPI does not depend on the actual monitor dpi but on gsettings *org.gnome.desktop.interface text-scaling-factor*. By default this value is 96, so font scaling is 1, it only jumps to 1.5 when the gtk settings of the scaling factor is more than 1.375. > > I think we still want to scale widgets by 1.5x when the font scale factor > 1.375 because if user wants to have bigger fonts, they probably want to have bigger widgets. > > [1] https://searchfox.org/mozilla-central/source/widget/gtk/nsWindow.cpp#825 > [2] https://searchfox.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#331 > [3] https://developer.gnome.org/gdk3/stable/GdkScreen.html#gdk-screen-get-resolution You're right, but it works as expected when text-scaling-factor is 1. We can't base widget sizes on font sizes, it produces incompatibility with rest of the system. Please use the same approach for widget sizes as Gtk+ does - use the gdk_window_get_scale_factor() only (probably at GetDefaultScaleInternal()). If user wants to override firefox scale it can be done by layout.css.devPixelsPerPx.
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219972 ::: widget/gtk/nsNativeThemeGTK.cpp:1060 (Diff revision 1) > return false; > } > default: > return false; > } > - gint scale = ScreenHelperGTK::GetGTKMonitorScaleFactor(); > + gint scale = GetThemeDpiScaleFactor(aFrame); Also is ScreenHelperGTK::GetGTKMonitorScaleFactor() useful in any situation? If not it should be removed/replaced on all places.
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219964 ::: widget/gtk/nsNativeThemeGTK.cpp:69 (Diff revision 1) > +static inline double > +GetThemeDpiScaleFactor(nsIFrame* aFrame) > +{ > + nsIWidget* rootWidget = aFrame->PresContext()->GetRootWidget(); > + if (rootWidget) { > + return rootWidget->GetDefaultScale().scale; I got it now, this can't be multiplied by scalefontfactor as long as this is relevant only for MS Windows.
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review220624 Please fix the double scale issue at nsNativeThemeGTK::GetMinimumWidgetSize().
Attachment #8943566 - Flags: review?(stransky)
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review219638 > This is already scaled so don't apply the scale again. I'm not going to agree with that, according to some measurement I just did, it needs to be scaled. Could you please clarify your statment?
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review220806
Attachment #8943566 - Flags: review?(stransky) → review+
Keywords: checkin-needed
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/016f4ce0803b Scale font for the actual monitor, not the first one; r=stransky https://hg.mozilla.org/integration/autoland/rev/c70e75e993fc Scale widget size to the current monitor, not the first one; r=stransky
Keywords: checkin-needed
Backed out 2 changesets (bug 1431337) for M15 failures in parser/htmlparser/tests/mochitest/test_img_picture_preload.html on a CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c70e75e993fcc25fb0c31d1fc137c5c25eef2ae0 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158216770&repo=autoland Backout: https://hg.mozilla.org/integration/autoland/rev/dcf7e3d2d3ab341dcb8c87bc61a29f8d9198ce37
Flags: needinfo?(jhorak)
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review221216
Comment on attachment 8943566 [details] Bug 1431337 Scale widget size to the current monitor, not the first one; https://reviewboard.mozilla.org/r/213916/#review221224 Removed r+ per discussion with patch author.
Attachment #8943566 - Flags: review+
Attachment #8943566 - Flags: review?(stransky)
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review222374 We need to fix scaling when font DPI is set to high values.
Attachment #8943565 - Flags: review+
Attachment #8943566 - Attachment is obsolete: true
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review223200 Looks like we have try failures here - bc5. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca8f3023f0ee
Attachment #8943565 - Flags: review?(stransky)
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try searching for changes remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '2595' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc' remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by 'hgssh4.dmz.scl3.mozilla.com/effffffc:2595' abort: stream ended unexpectedly (got 0 bytes, expected 4)
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review225624 ::: widget/gtk/WindowSurfaceWayland.cpp:607 (Diff revision 2) > WindowSurfaceWayland::GetBufferToDraw(int aWidth, int aHeight) > { > if (!mFrontBuffer) { > mFrontBuffer = new WindowBackBuffer(mWaylandDisplay, aWidth, aHeight); > mBackBuffer = new WindowBackBuffer(mWaylandDisplay, aWidth, aHeight); > return mFrontBuffer; We need to set scaling factor also for newly created buffers, right? ::: widget/gtk/WindowSurfaceWayland.cpp:646 (Diff revision 2) > } else { > // Former buffer has different size from the new request. Only resize > // the new buffer and leave gecko to render new whole content. > mFrontBuffer->Resize(aWidth, aHeight); > } > IMHO we also need to scale here when buffers are switched.
Attachment #8943565 - Flags: review?(stransky)
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review225636 r+ with those changes.
Attachment #8943565 - Flags: review?(stransky) → review+
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/cf44885dc2ac Scale content for the actual monitor, not the first one; r=stransky
Karl, I need some help with the failing test. I find out the showing of chevron.svg (ie test failing) is because my patch is actually fixing font sizes for layout.css.devPixelsPerPx=2. See screenshot for demonstration. The chevron symbol is shown during startup because the urlbar with bigger font size is considered to be too short. Is it fine to remove chevron.svg from the whitelist for the linux platform?
Flags: needinfo?(jhorak) → needinfo?(karlt)
I assume the urlbar font is a system font. If so, the font size in device pixels is determined by the system. On Wayland, I assume this depends on the monitor, but it should be independent from the value of layout.css.devPixelsPerPx. i.e. Changing layout.css.devPixelsPerPx should not change the appearance of system fonts. (There is an outstanding bug on the WINNT implementation.) Similarly widgets that have sizes determined by the system should be independent from layout.css.devPixelsPerPx. (Icons that have sizes provided by Firefox CSS px rather than the system will change size when layout.css.devPixelsPerPx changes.) IIRC LookAndFeel returns a font size in device pixels. I assume this will need to be for a reference monitor (potentially the primary) and the caller will adjust as appropriate for other monitors.
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #31) > IIRC LookAndFeel returns a font size in device pixels. I assume this will > need to be for a reference monitor (potentially the primary) and the caller > will adjust as appropriate for other monitors. Or perhaps they are display pixels, which is equivalent to a reference monitor with display pixels matching device pixels. I don't recall what the reference is.
(In reply to Karl Tomlinson (:karlt) from comment #31) > I assume the urlbar font is a system font. If so, the font size in device > pixels is determined by the system. On Wayland, I assume this depends on the > monitor, but it should be independent from the value of > layout.css.devPixelsPerPx. i.e. Changing layout.css.devPixelsPerPx should > not > change the appearance of system fonts. (There is an outstanding bug > on the WINNT implementation.) Oh, is it? It seems to me that it is inconsistent ATM, if you look at the left part of screenshot the page content actually depends on devPixelsPerPx but the UI fonts does not. I supposed that devPixelsPerPx can be used to force 2x scaling for hidpi monitors. I've switched to Win 10 to check how things work there with devPixelsPerPx=2 and it seems everything is scaled 2x like in the patch I did. I'll attach screenshot later.
Attached image Win 10 with devPixelsPerPx-2.png (deleted) —
Comment on attachment 8952386 [details] Bug 1431337 - Skip hidpi test of startup images because they overflow in Linux https://reviewboard.mozilla.org/r/221632/#review227530
Attachment #8952386 - Flags: review+
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/f969c4ecfd05 Scale content for the actual monitor, not the first one; r=stransky https://hg.mozilla.org/integration/autoland/rev/e8b95716f05c Skip hidpi test of startup images because they overflow in Linux r=stransky
https://hg.mozilla.org/mozilla-central/rev/f969c4ecfd05 https://hg.mozilla.org/mozilla-central/rev/e8b95716f05c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(In reply to Jan Horak [:jhorak] from comment #33) > It seems to me that it is inconsistent ATM, if you look at the > left part of screenshot the page content actually depends on devPixelsPerPx > but the UI fonts does not. The left part of the screenshot is correct. devPixelsPerPx defines how large a CSS pixel is, but the UI fonts are the size of system fonts, which are not defined in CSS pixels. > I supposed that devPixelsPerPx can be used to > force 2x scaling for hidpi monitors. Yes, that can be used to set the scaling of web content and that also happens to affect UI elements that do not depend on system sizes. But system fonts should still be the same size as system fonts. If a user wants to double the size of a CSS pixel, then they are likely to have already doubled the size of system fonts, and so system font sizes should not be doubled again. If the goal is to change the size of everything, then GDK_SCALE and GDK_DPI_SCALE can be used to scale the system fonts as well as the size of CSS pixels when the devPixelsPerPx pref is at it's default of -1. Scaling everything is not the purpose of devPixelsPerPx. > I've switched to Win 10 to check how > things work there with devPixelsPerPx=2 and it seems everything is scaled 2x > like in the patch I did. I'll attach screenshot later. Yes, there is a bug on file for that.
(In reply to Karl Tomlinson (:karlt) from comment #41) > > I supposed that devPixelsPerPx can be used to > > force 2x scaling for hidpi monitors. > > Yes, that can be used to set the scaling of web content and that also happens > to affect UI elements that do not depend on system sizes. > > But system fonts should still be the same size as system fonts. If a user > wants to double the size of a CSS pixel, then they are likely to have already > doubled the size of system fonts, and so system font sizes should not be > doubled again. > > If the goal is to change the size of everything, then GDK_SCALE and > GDK_DPI_SCALE can be used to scale the system fonts as well as the > size of CSS pixels when the devPixelsPerPx pref is at it's default of -1. > Scaling everything is not the purpose of devPixelsPerPx. > > > I've switched to Win 10 to check how > > things work there with devPixelsPerPx=2 and it seems everything is scaled 2x > > like in the patch I did. I'll attach screenshot later. > > Yes, there is a bug on file for that. I suggest to keep the recent patch and file a follow up bug. While it's not perfect it still dramatically improves Firefox experience for users with HiDPI (or semi-HiDPI) displays.
When disabling tests, please seek review from the owner of the test. This actually applies to all file changes: the reviewer should be qualified in the area appropriate for the change. If the test should be disabled then the owner should know, so that they can prioritize fixing the test. I assume the test will be re-enabled as part of fixing bug 1439857, but just adding needinfo to ensure you see the procedures used in this project.
Flags: needinfo?(jhorak)
(In reply to Karl Tomlinson (:karlt) from comment #41) > (In reply to Jan Horak [:jhorak] from comment #33) > > It seems to me that it is inconsistent ATM, if you look at the > > left part of screenshot the page content actually depends on devPixelsPerPx > > but the UI fonts does not. > > The left part of the screenshot is correct. > devPixelsPerPx defines how large a CSS pixel is, but the UI fonts are > the size of system fonts, which are not defined in CSS pixels. > > > I supposed that devPixelsPerPx can be used to > > force 2x scaling for hidpi monitors. > > Yes, that can be used to set the scaling of web content and that also happens > to affect UI elements that do not depend on system sizes. Thanks for clarification. Looking at the UI elements, when devPixelsPerPx is set, should we scale GTK UI elements as well (like scrollbars, etc)? Pre-patch behaviour is not to scale them (check the scrollbar on the screenshot). > But system fonts should still be the same size as system fonts. If a user > wants to double the size of a CSS pixel, then they are likely to have already > doubled the size of system fonts, and so system font sizes should not be > doubled again. > > If the goal is to change the size of everything, then GDK_SCALE and > GDK_DPI_SCALE can be used to scale the system fonts as well as the > size of CSS pixels when the devPixelsPerPx pref is at it's default of -1. > Scaling everything is not the purpose of devPixelsPerPx. > > > I've switched to Win 10 to check how > > things work there with devPixelsPerPx=2 and it seems everything is scaled 2x > > like in the patch I did. I'll attach screenshot later. > > Yes, there is a bug on file for that. I see, I saw it as a reference implementation for us. > I assume the test will be re-enabled as part of fixing bug 1439857, > but just adding needinfo to ensure you see the procedures used in > this project. That is the plan, keeping needinfo there.
No longer blocks: 1439881
Depends on: 1439881
Wontfix?
(In reply to Jan Horak [:jhorak] from comment #44) > (In reply to Karl Tomlinson (:karlt) from comment #41) > > Yes, that can be used to set the scaling of web content and that also happens > > to affect UI elements that do not depend on system sizes. > Thanks for clarification. Looking at the UI elements, when devPixelsPerPx is > set, should we scale GTK UI elements as well (like scrollbars, etc)? > Pre-patch behaviour is not to scale them (check the scrollbar on the > screenshot). For scrollbars and other UI elements where the sizes come from GTK APIs, sizes should depend on the monitor scale but not devPixelsPerPx. This provides that native UI elements look like native UI elements. The system theme and GTK settings determine the sizes. Some UI elements may be sized in CSS pixels, either because they do not have native intrinsic or minimum sizes or because the size is provided by the document. Their sizes will depend devPixelsPerPx because the size of a CSS pixel depends on devPixelsPerPx. (In reply to luis.pabon from comment #45) > Wontfix? That just means that this fix will be shipped in version 60, not version 59.
I just upgraded to 60b3 and this problem still exists
(In reply to khc from comment #47) > I just upgraded to 60b3 and this problem still exists This is wayland only fix, does not apply to standard X11 builds provided by Mozilla.
Doesn't work for me with 60.0~b11+build2-0ubuntu0.17.10.1 installed from the Beta PPA. I'm on Wayland.

This seems to have regressed for me in Firefox 64.0.2...

Hmm, also seems broken if I downgrade to 62.0.3... I could have sworn that it worked fine recently. Perhaps something else changed on my system (like XWayland)...?

Also broken for me using Ubuntu 18.04.1 LTS with Wayland.
I tried using 64.0+build3-0ubuntu0.18.04.1 and 66.0a1hg20190128r455577-0ubuntu0.18.04.1~umd1

Seening this still on Fedora 30 (still beta)... though becavior is inconsistant... right-clicking on icons in the toolbar might or might not pop out a menu in the wrong side.

Sharing my laptp 3200x1800 (16:9) to desktop which has 1920x1080 (16:9).

Installed using the officaly firefox-wayland avaiable for Fedora 30... firefox-wayland-66.0.2-1.fc30.x86_64

(In reply to us.social from comment #53)

Seening this still on Fedora 30 (still beta)... though becavior is
inconsistant... right-clicking on icons in the toolbar might or might not
pop out a menu in the wrong side.

Sharing my laptp 3200x1800 (16:9) to desktop which has 1920x1080 (16:9).

Installed using the officaly firefox-wayland avaiable for Fedora 30...
firefox-wayland-66.0.2-1.fc30.x86_64

Please file a bug at bugzilla.redhat.com. The firefox-wayland-66.0.2-1.fc30.x86_64 is not relevant here.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: