Closed Bug 1431337 Opened 5 years ago Closed 5 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;
59 bytes, text/x-review-board-request
stransky
: review+
Details
chevron-layout.css.devPixelsPerPx-2-issue.png
1.25 MB, image/png
Details
Win 10 with devPixelsPerPx-2.png
1.88 MB, image/png
Details
Bug 1431337 - Skip hidpi test of startup images because they overflow in Linux
59 bytes, 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.

    
    

    
      
  

    
    

    
  
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: 5 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.
Depends on: 1439857

    
    

    
  
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.