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

[LBSE] Computation of on-screen font scaling factor ignores page zoom #15000

Conversation

nikolaszimmermann
Copy link
Contributor

@nikolaszimmermann nikolaszimmermann commented Jun 15, 2023

988fed7

[LBSE] Computation of on-screen font scaling factor ignores page zoom
https://bugs.webkit.org/show_bug.cgi?id=258120

Reviewed by Rob Buis.

For standalone SVG documents the page zoom has to be taken into account when computing the
on-screen font scaling factor. Currently we ignore the effective zoom factor and thus select
a sub-optimal font.

Non-scaling-stroke (webkit.org/b/139322) works properly in LBSE, if this fix is applied as well.
Two tests fail in LBSE without this patch: svg/text/non-scaling-stroke-textRendering-default.svg
and svg/text/non-scaling-stroke-textRendering-geometricPrecision.svg).

Covered by new tests added by webkit.org/b/139322 -- easy to test with non-scaling-stroke.
Without non-scaling-stroke, this is most noticeable in zoom-zoom-coords.html which now
exposes slighly different metrics for <text> since the internal font-size scaling factor
changed (now taking zoom into account, as the legacy SVG engine already does).

zoom-zoom-coords.xhtml now delivers correct results across all platforms and is identical
for legacy SVG engine and LBSE -- get rid of platform specific results, finally.

* LayoutTests/platform/glib/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/platform/mac-ventura-wk2-lbse-text/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/platform/mac/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:
Finally a single non-LBSE/LBSE result for all platforms.
* LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:
* Source/WebCore/rendering/svg/SVGLayerTransformComputation.h:
(WebCore::SVGLayerTransformComputation::calculateScreenFontSizeScalingFactor const):
Multiply scaling factor by effectiveZoom() (which includes page zoom), but only for non-standalone
SVG documents. For standalone SVG documents the page-zoom induced scaling is already part of the CTM.

Canonical link: https://commits.webkit.org/265208@main

5eed14e

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 βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@nikolaszimmermann nikolaszimmermann self-assigned this Jun 15, 2023
@nikolaszimmermann nikolaszimmermann added the SVG For bugs in the SVG implementation. label Jun 15, 2023
@nikolaszimmermann
Copy link
Contributor Author

The 100th LBSE patch (see nikolaszimmermann/WebKitIgalia#1) got a nice PR number: 15000 :-)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 15, 2023
Copy link
Contributor

@rwlbuis rwlbuis left a comment

Choose a reason for hiding this comment

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

Looks good, but it seems zoom-zoom-coords.xhtml needs fixing.

@nikolaszimmermann
Copy link
Contributor Author

Yop, will take care of it later, thanks @rwlbuis!

@nikolaszimmermann nikolaszimmermann removed the merging-blocked Applied to prevent a change from being merged label Jun 15, 2023
@nikolaszimmermann nikolaszimmermann force-pushed the eng/LBSE-Computation-of-on-screen-font-scaling-factor-ignores-page-zoom branch from 469e0f7 to 5eed14e Compare June 15, 2023 18:39
@nikolaszimmermann
Copy link
Contributor Author

Hah, zoom-zoom-coords.xhtml delivers the same results for LBSE and the legacy SVG engine, both across all platforms! :-) Can finally remove the platform specific results for that tricky test case.

@nikolaszimmermann nikolaszimmermann added the merge-queue Applied to send a pull request to merge-queue label Jun 15, 2023
https://bugs.webkit.org/show_bug.cgi?id=258120

Reviewed by Rob Buis.

For standalone SVG documents the page zoom has to be taken into account when computing the
on-screen font scaling factor. Currently we ignore the effective zoom factor and thus select
a sub-optimal font.

Non-scaling-stroke (webkit.org/b/139322) works properly in LBSE, if this fix is applied as well.
Two tests fail in LBSE without this patch: svg/text/non-scaling-stroke-textRendering-default.svg
and svg/text/non-scaling-stroke-textRendering-geometricPrecision.svg).

Covered by new tests added by webkit.org/b/139322 -- easy to test with non-scaling-stroke.
Without non-scaling-stroke, this is most noticeable in zoom-zoom-coords.html which now
exposes slighly different metrics for <text> since the internal font-size scaling factor
changed (now taking zoom into account, as the legacy SVG engine already does).

zoom-zoom-coords.xhtml now delivers correct results across all platforms and is identical
for legacy SVG engine and LBSE -- get rid of platform specific results, finally.

* LayoutTests/platform/glib/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/platform/mac-ventura-wk2-lbse-text/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/platform/mac/svg/zoom/page/zoom-zoom-coords-expected.txt: Removed.
* LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:
Finally a single non-LBSE/LBSE result for all platforms.
* LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:
* Source/WebCore/rendering/svg/SVGLayerTransformComputation.h:
(WebCore::SVGLayerTransformComputation::calculateScreenFontSizeScalingFactor const):
Multiply scaling factor by effectiveZoom() (which includes page zoom), but only for non-standalone
SVG documents. For standalone SVG documents the page-zoom induced scaling is already part of the CTM.

Canonical link: https://commits.webkit.org/265208@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/LBSE-Computation-of-on-screen-font-scaling-factor-ignores-page-zoom branch from 5eed14e to 988fed7 Compare June 15, 2023 19:38
@webkit-commit-queue
Copy link
Collaborator

Committed 265208@main (988fed7): https://commits.webkit.org/265208@main

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

@webkit-commit-queue webkit-commit-queue merged commit 988fed7 into WebKit:main Jun 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 15, 2023
@nikolaszimmermann nikolaszimmermann deleted the eng/LBSE-Computation-of-on-screen-font-scaling-factor-ignores-page-zoom branch June 15, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SVG For bugs in the SVG implementation.
Projects
None yet
5 participants