[hidpi] [wayland] moving window between hidpi and normal dpi monitor does not scale content correctly
Categories
(Core :: Widget: Gtk, defect, P3)
Tracking
()
People
(Reporter: jhorak, Assigned: jhorak, NeedInfo)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8943565 [details] Bug 1431337 - Scale content for the actual monitor, not the first one; https://reviewboard.mozilla.org/r/213914/#review219636
Comment 4•6 years ago
|
||
mozreview-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.
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-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 6•6 years ago
|
||
mozreview-review-reply |
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 7•6 years ago
|
||
mozreview-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/#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.
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-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/#review220624 Please fix the double scale issue at nsNativeThemeGTK::GetMinimumWidgetSize().
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-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/#review220806
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
mozreview-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/#review221216
Comment 18•6 years ago
|
||
mozreview-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/#review221224 Removed r+ per discussion with patch author.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Comment 20•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Comment 23•6 years ago
|
||
mozreview-review |
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
Comment 24•6 years ago
|
||
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 25•6 years ago
|
||
mozreview-review |
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.
Comment 26•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment 28•6 years ago
|
||
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
Comment 29•6 years ago
|
||
Baked out for failing mochitest browser chrome at browser/base/content/test/performance/browser_startup_images.js Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cf44885dc2aca866ed9345b28337d146b02e1371 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162408183&repo=autoland&lineNumber=2787 Backout: https://hg.mozilla.org/integration/autoland/rev/3ffcad22d632d1cd97285b3a54cb469227bd1df1
Assignee | ||
Comment 30•6 years ago
|
||
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?
Comment 31•6 years ago
|
||
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.
Comment 32•6 years ago
|
||
(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.
Assignee | ||
Comment 33•6 years ago
|
||
(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.
Assignee | ||
Comment 34•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•6 years ago
|
||
Try run with ignoring hidpi part of the test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11e85011e495059ec93956089e12207a529ab55d&selectedJob=163206310
Comment 38•6 years ago
|
||
mozreview-review |
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
Comment 39•6 years ago
|
||
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
Comment 40•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f969c4ecfd05 https://hg.mozilla.org/mozilla-central/rev/e8b95716f05c
Comment 41•6 years ago
|
||
(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.
Comment 42•6 years ago
|
||
(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.
Comment 43•6 years ago
|
||
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.
Assignee | ||
Comment 44•6 years ago
|
||
(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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 45•6 years ago
|
||
Wontfix?
Comment 46•6 years ago
|
||
(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.
Comment 47•6 years ago
|
||
I just upgraded to 60b3 and this problem still exists
Comment 48•6 years ago
|
||
(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.
Comment 49•6 years ago
|
||
Doesn't work for me with 60.0~b11+build2-0ubuntu0.17.10.1 installed from the Beta PPA. I'm on Wayland.
Comment 50•5 years ago
|
||
This seems to have regressed for me in Firefox 64.0.2...
Comment 51•5 years ago
|
||
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)...?
Comment 52•5 years ago
|
||
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
Comment 53•5 years ago
|
||
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
Comment 54•5 years ago
|
||
(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.
Description
•