Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GTK] Correct scaling for DPI. #26938

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Conversation

gwhitney
Copy link
Contributor

@gwhitney gwhitney commented Apr 6, 2024

69ad530

[GTK] Correct scaling for DPI.
https://bugs.webkit.org/show_bug.cgi?id=247980

Reviewed by NOBODY (OOPS!).

Move the scaling factor formerly in WebKitWebView.cpp (responsive to
the xft-dpi setting) into WebKitWebViewBase.cpp, and split it into two
scaling factors. The first, a page scaling factor, uses screenDPI to
ensure that a length specified by a CSS unit of 1in will appear on the
screen as one physical inch. The second, a text scaling factor, ensures
that text specified as 96px in size will have a height on-screen as
specified by fontDPI (which is primarily driven by the xft-dpi setting),
in terms of the number of screen pixels taken up.

The two scaling factors are refreshed when any one of the inputs for
computing them changes: the GDK_SCALE factor, the xft-dpi setting, or
the screenDPI of the device it is displayed on (say by being dragged
to a different monitor). Throughout, at the default zoom level, a CSS
1in dimension is kept at one physical inch.

The scale factors are kept internal to WebKitWebViewBase (which helps
keep GTK dependencies grouped and reduces the total code in WebKitWebView
enclosed in `#ifdef PLATFORM(GTK)`) but made available to WebKitWebView
so that its external interfaces can maintain zoom level 1 as the default
zoom level, and zoom in and out from there.

So indeed, if a user of WebKitWebView asks for a zoom level of 1.5, a
length specified in CSS as 1in will then occupy one and a half physical
inches on screen.

* Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:
(WebCore::fontDPI):
* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
(webkitWebViewConstructed):
(webkitWebViewDispose):
(webkit_web_view_set_zoom_level):
(webkit_web_view_get_zoom_level):
* Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:
(_WebKitWebViewBasePrivate::_WebKitWebViewBasePrivate):
(refreshInternalScaling):
(webkitWebViewBaseUpdateDisplayID):
(webkitWebViewBaseDispose):
(webkitWebViewBaseGetScaleFactors):
(deviceScaleFactorChanged):
(webkitWebViewBaseCreateWebPage):
* Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:

69ad530

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ api-wpe
  πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-skia
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2   πŸ§ͺ gtk-wk2
  πŸ›  tv-sim   πŸ§ͺ mac-wk2-stress   πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
❌ πŸ›  watch-sim

@gwhitney gwhitney requested review from zdobersek, a team and cdumez as code owners April 6, 2024 20:17
@gwhitney gwhitney changed the title Correct scaling for DPI in under GTK. Correct scaling for DPI under GTK. Apr 6, 2024
@gwhitney gwhitney changed the title Correct scaling for DPI under GTK. [GTK] Correct scaling for monitor and Xft DPI. Apr 6, 2024
@mcatanzaro
Copy link
Contributor

Hi, we'll look closer at this next week, but for now please start by splitting this into two pull requests because unfortunately each pull request has to have exactly one commit in order to land. Probably best to squash that second commit into the first one.

I'm going to mark this one as Draft for now. You can create a separate pull request with just one commit (corresponding to the first two commits from this pull request). Then after that pull request lands, you can rebase this one such that it contains only the third commit, and remove the Draft status.

@gwhitney
Copy link
Contributor Author

gwhitney commented Apr 7, 2024

OK,will do ASAP.

@gwhitney
Copy link
Contributor Author

gwhitney commented Apr 7, 2024

Filed #26954.

@mcatanzaro
Copy link
Contributor

Reminder: after #26954 lands, rebase this pull request and then ping @WebKit/glib-reviewers to re-request review.

@gwhitney
Copy link
Contributor Author

OK, I see that #26954 has landed. I will rebase/update this pull request as soon as I am able and mention the handle you gave when it is done. Thanks.

@gwhitney gwhitney changed the title [GTK] Correct scaling for monitor and Xft DPI. [GTK] Correct scaling for DPI. Apr 17, 2024
@gwhitney
Copy link
Contributor Author

This bugfix has been updated with respect to the further discussion in https://bugs.webkit.org/show_bug.cgi?id=247980 and rebased on top of #26954. I believe it resolves the associated issue, and would welcome @bubbleguuum to test it if possible. Thanks in advance for reviewing, @WebKit/glib-reviewers.

@mcatanzaro mcatanzaro added the GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch label Apr 18, 2024
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Just a few nits regarding style.

Source/WebCore/platform/gtk/PlatformScreenGtk.cpp Outdated Show resolved Hide resolved
Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp Outdated Show resolved Hide resolved
Source/WebCore/platform/gtk/PlatformScreenGtk.cpp Outdated Show resolved Hide resolved
@gwhitney gwhitney marked this pull request as draft April 18, 2024 21:49
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment in https://bugs.webkit.org/show_bug.cgi?id=250138. I'm going to click the Approve button. Time to update with my suggested changes and mark as ready rather than Draft.

@gwhitney
Copy link
Contributor Author

OK, I will amend the commit as requested and unmark as draft within the next 24 hours. Thanks!

@gwhitney gwhitney marked this pull request as ready for review April 29, 2024 19:36
@gwhitney
Copy link
Contributor Author

Sorry about the delay resulting from the conversation on webkit bugzilla 250138. Hope this is ready to go now, even if that conversation was unsatisfactory.

Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this breaks the Large Text accessibility setting, see https://bugs.webkit.org/show_bug.cgi?id=250138. We'll need to figure this out before landing.

Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, sorry... I failed to test whether it worked without changes, and it doesn't. Seems the large text setting is only respected when running in flatpak not respected if I fail to change it properly. Let's not block on this.

@mcatanzaro mcatanzaro added the merge-queue Applied to send a pull request to merge-queue label Apr 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=247980

Reviewed by Michael Catanzaro.

Move the scaling factor formerly in WebKitWebView.cpp (responsive to
the xft-dpi setting) into WebKitWebViewBase.cpp, and split it into two
scaling factors. The first, a page scaling factor, uses screenDPI to
ensure that a length specified by a CSS unit of 1in will appear on the
screen as one physical inch. The second, a text scaling factor, ensures
that text specified as 96px in size will have a height on-screen as
specified by fontDPI (which is primarily driven by the xft-dpi setting),
in terms of the number of screen pixels taken up.

The two scaling factors are refreshed when any one of the inputs for
computing them changes: the GDK_SCALE factor, the xft-dpi setting, or
the screenDPI of the device it is displayed on (say by being dragged
to a different monitor). Throughout, at the default zoom level, a CSS
1in dimension is kept at one physical inch.

The scale factors are kept internal to WebKitWebViewBase (which helps
keep GTK dependencies grouped and reduces the total code in WebKitWebView
enclosed in `#ifdef PLATFORM(GTK)`) but made available to WebKitWebView
so that its external interfaces can maintain zoom level 1 as the default
zoom level, and zoom in and out from there.

So indeed, if a user of WebKitWebView asks for a zoom level of 1.5, a
length specified in CSS as 1in will then occupy one and a half physical
inches on screen.

* Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:
(WebCore::fontDPI):
* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
(webkitWebViewConstructed):
(webkitWebViewDispose):
(webkit_web_view_set_zoom_level):
(webkit_web_view_get_zoom_level):
* Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:
(_WebKitWebViewBasePrivate::_WebKitWebViewBasePrivate):
(refreshInternalScaling):
(webkitWebViewBaseUpdateDisplayID):
(webkitWebViewBaseDispose):
(webkitWebViewBaseGetScaleFactors):
(deviceScaleFactorChanged):
(webkitWebViewBaseCreateWebPage):
* Source/WebKit/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:

Canonical link: https://commits.webkit.org/278128@main
@webkit-commit-queue
Copy link
Collaborator

Committed 278128@main (eb01eca): https://commits.webkit.org/278128@main

Reviewed commits have been landed. Closing PR #26938 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit eb01eca into WebKit:main Apr 29, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 29, 2024
@aperezdc
Copy link
Contributor

aperezdc commented May 2, 2024

Backported into the 2.44 branch as commit 9864937

@aperezdc aperezdc removed the GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants