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

Account for zoom when computing SVG font size #4011

Closed
wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Sep 4, 2022

Account for zoom when computing SVG font size

https://bugs.webkit.org/show_bug.cgi?id=118818

Reviewed by NOBODY (OOPS!).

Merge - https://chromium.googlesource.com/chromium/blink/+/5624c6e475af92d541edc8a3394602e320d00323 & https://src.chromium.org/viewvc/blink?view=revision&revision=180995 & https://src.chromium.org/viewvc/blink?view=revision&revision=154004

Previously, text that was zoomed appeared larger than geometry under the same zoom value. This was caused by two bugs:
1) When setting the computed font size, we incorrectly referenced the computedTextSize instead of the specified size.
2) Text with GeometricPrecision did not set a computed size but instead relied on the text size being correct.

Further, it also fixes an issue about optimization in RenderSVGInlineText::computeNewScaledFontForStyle would skip updating the font size on lowdpi devices while zooming. The computed font size calculation depends on zoom and the font scaling factor depends on the device's scaling factor. Previously, we would skip updating the font when the scaling factor was 1 which would fail to account for zoom. This patch updates the optimization to only be used when both the scaling factor and zoom are 1.

This change addresses both of these issues and adds a test to prove correctness.

SVG doesn't scale zoomed font units so zooming should not affect font sizes. This patch fixes a bug where em lengths were resolved against the style's font size (with zoom) instead of the specified font size (without zoom).

* Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:
(RenderSVGInlineText::computedNewScaledFontForStyle) - Updated Scaling factor to be accounted into two buckets: 1) GeometricPrecision 2) computedTextSize being referred 3) Use DoNotUseSmartMinimumForFontSize 4) Also fix low DPI issue
* Source/WebCore/svg/SVGLengthContext.cpp: Update "convertValueFromUserUnitsToEMS" and "convertValueFromEMSToUserUnits" to use "specifiedFontSize" rather than "computedFontPixelSize"
* LayoutTests/svg/zoom/text/zoom-text-geometry.html: Added Test Case
* LayoutTests/svg/zoom/text/zoom-text-geometry-expected.html: Added Test Case Expectations
* LayoutTests/svg/zoom/text/lowdpi-zoom-text.html: Added Test Case
* LayoutTests/svg/texst/lowdpi-zoom-text-expected.txt: Added Test Case Expectations
* LayoutTets/svg/zoom/text/zoom-em-units.html: Added Test Case
* LayoutTets/svg/zoom/text/zoom-em-units-expected.html: Added Test Case Expectations

488e2eb

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 5, 2022
@Ahmad-S792
Copy link
Contributor Author

@smfr & @nikolaszimmermann - Can you guide or suggest on how to fix this 'ios-wk2' failure (svg/text/font-small-enlarged-minimum-larger.svg)?

@Ahmad-S792 Ahmad-S792 force-pushed the fix118818 branch 2 times, most recently from 971c72d to dd62d17 Compare September 10, 2022 19:38
@nikolaszimmermann
Copy link
Contributor

Sorry for not responding at all - I'm pretty busy these days...

Copy link
Contributor

@nikolaszimmermann nikolaszimmermann 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 tackling this -- not ready though, the LayoutTests changes seem fishy.

@Ahmad-S792
Copy link
Contributor Author

Thanks for tackling this -- not ready though, the LayoutTests changes seem fishy.

Replied above and would appreciate your guidance because I am able to pass all changes by adding 'font-size: 80' but only fails in iOS-wk2 and have asked for input from Simon and he referred that it might be iOS device scaling factor and I should look into hidpi test cases.

Copy link
Contributor

@nikolaszimmermann nikolaszimmermann left a comment

Choose a reason for hiding this comment

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

That looks good - let's wait for EWS results. If ok, I'll r+ it. Thanks @Ahmad-S792 for your patience.

@Ahmad-S792
Copy link
Contributor Author

That looks good - let's wait for EWS results. If ok, I'll r+ it. Thanks @Ahmad-S792 for your patience.

No worries - I should appreciate you to deal with noob like me. :-)

I am just learning day by day and have day job so some time response take some time. :-)

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Sep 21, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 22, 2022
Account for zoom when computing SVG font size

https://bugs.webkit.org/show_bug.cgi?id=118818

Reviewed by NOBODY (OOPS!).

Merge - https://chromium.googlesource.com/chromium/blink/+/5624c6e475af92d541edc8a3394602e320d00323 & https://src.chromium.org/viewvc/blink?view=revision&revision=180995 & https://src.chromium.org/viewvc/blink?view=revision&revision=154004

Previously, text that was zoomed appeared larger than geometry under the same zoom value. This was caused by two bugs:
1) When setting the computed font size, we incorrectly referenced the computedTextSize instead of the specified size.
2) Text with GeometricPrecision did not set a computed size but instead relied on the text size being correct.

Further, it also fixes an issue about optimization in RenderSVGInlineText::computeNewScaledFontForStyle would skip updating the font size on lowdpi devices while zooming. The computed font size calculation depends on zoom and the font scaling factor depends on the device's scaling factor. Previously, we would skip updating the font when the scaling factor was 1 which would fail to account for zoom. This patch updates the optimization to only be used when both the scaling factor and zoom are 1.

This change addresses both of these issues and adds a test to prove correctness.

SVG doesn't scale zoomed font units so zooming should not affect font sizes. This patch fixes a bug where em lengths were resolved against the style's font size (with zoom) instead of the specified font size (without zoom).

* Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:
(RenderSVGInlineText::computedNewScaledFontForStyle) - Updated Scaling factor to be accounted into two buckets: 1) GeometricPrecision 2) computedTextSize being referred 3) Use DoNotUseSmartMinimumForFontSize 4) Also fix low DPI issue
* Source/WebCore/svg/SVGLengthContext.cpp: Update "convertValueFromUserUnitsToEMS" and "convertValueFromEMSToUserUnits" to use "specifiedFontSize" rather than "computedFontPixelSize"
* LayoutTests/svg/zoom/text/zoom-text-geometry.html: Added Test Case
* LayoutTests/svg/zoom/text/zoom-text-geometry-expected.html: Added Test Case Expectations
* LayoutTests/svg/zoom/text/lowdpi-zoom-text.html: Added Test Case
* LayoutTests/svg/texst/lowdpi-zoom-text-expected.txt: Added Test Case Expectations
* LayoutTets/svg/zoom/text/zoom-em-units.html: Added Test Case
* LayoutTets/svg/zoom/text/zoom-em-units-expected.html: Added Test Case Expectations
@Ahmad-S792
Copy link
Contributor Author

As discussed with @nikolaszimmermann over Slack, there is another change, which we require prior to landing this. So I will work on it and then have similar to this another PR to land this in future hoping that EWS does not reflect same failures.

@Ahmad-S792 Ahmad-S792 closed this Oct 17, 2022
@Ahmad-S792 Ahmad-S792 deleted the fix118818 branch December 9, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merging-blocked Applied to prevent a change from being merged SVG For bugs in the SVG implementation.
Projects
None yet
4 participants