Bug 191834 - Extremely small monospace text size when SVG is included as an img
Summary: Extremely small monospace text size when SVG is included as an img
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 12
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-19 05:31 PST by Derk-Jan Hartman
Modified: 2018-12-10 02:37 PST (History)
9 users (show)

See Also:


Attachments
HTML fragment (8.23 KB, text/html)
2018-11-19 05:32 PST, Derk-Jan Hartman
no flags Details
original svg (3.89 KB, image/svg+xml)
2018-11-19 05:32 PST, Derk-Jan Hartman
no flags Details
screenshot (26.62 KB, image/png)
2018-11-19 05:33 PST, Derk-Jan Hartman
no flags Details
WebKitGTK+ 2.22.2 screenshot (38.78 KB, image/png)
2018-11-19 06:58 PST, Peter Krautzberger
no flags Details
Patch (3.71 KB, patch)
2018-11-20 17:04 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.82 MB, application/zip)
2018-11-20 19:14 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Derk-Jan Hartman 2018-11-19 05:31:50 PST
When I have (monospace) Chinese characters in an SVG, they are not rendered when this SVG is included using the <img> tag, but are rendered just fine when directly included in the webpage.

Not sure if this is 'technically' correct behavior with font fallbacks and what not, but from a user's perspective, it is stupid and this difference shouldn't exist.

testcase included, incl. screenshot.

Safari 12
Comment 1 Derk-Jan Hartman 2018-11-19 05:32:40 PST
Created attachment 355272 [details]
HTML fragment
Comment 2 Derk-Jan Hartman 2018-11-19 05:32:58 PST
Created attachment 355273 [details]
original svg
Comment 3 Derk-Jan Hartman 2018-11-19 05:33:17 PST
Created attachment 355274 [details]
screenshot
Comment 4 Peter Krautzberger 2018-11-19 06:58:38 PST
Created attachment 355279 [details]
WebKitGTK+ 2.22.2 screenshot

On WebKitGTK+ 2.22.2, the text content is visible but is extremely small (i.e., unreadable).
Comment 5 Simon Fraser (smfr) 2018-11-20 15:57:11 PST
Seems to be triggered by the font-family='monospace' on the <text>. Doesn't happen if some other font is specified.
Comment 6 Simon Fraser (smfr) 2018-11-20 16:21:54 PST
In the bad case (SVG in <img>), we call fontSizeForKeyword() here:

  * frame #0: 0x0000000109530b64 WebCore`WebCore::Style::fontSizeForKeyword(keywordID=66, shouldUseFixedDefaultSize=true, document={ origin = Unique, url = , inMainFrame = 1, pageCacheState = NotInPageCache }) at StyleFontSizeFunctions.cpp:160
    frame #1: 0x000000010736b8da WebCore`WebCore::StyleBuilderCustom::applyValueFontFamily(styleResolver=0x0000000124862000, value=0x00000001248a5600) at StyleBuilderCustom.h:1019
    frame #2: 0x0000000107361ef7 WebCore`WebCore::StyleBuilder::applyProperty(property=CSSPropertyFontFamily, styleResolver=0x0000000124862000, value=0x00000001248a5600, isInitial=false, isInherit=false, registered=0x0000000000000000) at StyleBuilder.cpp:5491
    frame #3: 0x0000000107de3247 WebCore`WebCore::StyleResolver::applyProperty(this=0x0000000124862000, id=CSSPropertyFontFamily, value=0x00000001248a5600, applyState=0x00007ffeefbf0980, linkMatchMask=MatchDefault) at StyleResolver.cpp:1721
    frame #4: 0x0000000107de7f83 WebCore`WebCore::StyleResolver::CascadedProperties::Property::apply(this=0x00007ffeefbf0af8, resolver=0x0000000124862000, applyState=0x00007ffeefbf0980) at StyleResolver.cpp:2279
    frame #5: 0x0000000107de8550 WebCore`void WebCore::StyleResolver::applyCascadedPropertiesImpl<(WebCore::StyleResolver::CustomPropertyCycleTracking)1>(this=0x0000000124862000, firstProperty=2, lastProperty=29, state=0x00007ffeefbf0980) at StyleResolver.cpp:2378

and document.settings().defaultFixedFontSize() returns 0 (document.settings().defaultFontSize() is 16). So fontSizeForKeyword() returns 1, hence the tiny text.
Comment 7 Simon Fraser (smfr) 2018-11-20 16:28:16 PST
Myles, any idea why the defaultFixedFontSize default in Settings.yaml is 0, not a sensible value like 13?
Comment 8 Simon Fraser (smfr) 2018-11-20 17:04:45 PST
Created attachment 355372 [details]
Patch
Comment 9 Build Bot 2018-11-20 19:14:39 PST
Comment on attachment 355372 [details]
Patch

Attachment 355372 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10093898

New failing tests:
svg/text/monospace-text-size-in-img.html
Comment 10 Build Bot 2018-11-20 19:14:51 PST
Created attachment 355378 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Myles C. Maxfield 2018-11-21 12:04:52 PST
Comment on attachment 355372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355372&action=review

> Source/WebCore/ChangeLog:9
> +        who omit to set it (like SVGImages) don't get broken rendering.

Do we want to make SVGImages get explicit settings, instead of just getting whatever the defaults are?
Comment 12 Simon Fraser (smfr) 2018-11-21 16:04:26 PST
(In reply to Myles C. Maxfield from comment #11)
> Comment on attachment 355372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355372&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        who omit to set it (like SVGImages) don't get broken rendering.
> 
> Do we want to make SVGImages get explicit settings, instead of just getting
> whatever the defaults are?

Yes, I think we want this change and another change that pushes the values into SVGImage's settings.
Comment 13 Simon Fraser (smfr) 2018-11-22 09:45:10 PST
(In reply to Simon Fraser (smfr) from comment #12)
> (In reply to Myles C. Maxfield from comment #11)
> > Comment on attachment 355372 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355372&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        who omit to set it (like SVGImages) don't get broken rendering.
> > 
> > Do we want to make SVGImages get explicit settings, instead of just getting
> > whatever the defaults are?
> 
> Yes, I think we want this change and another change that pushes the values
> into SVGImage's settings.

Actually that's too hard; the SVGImage doesn't have an easy way to get to the enclosing Document. Let's go with this patch for now.
Comment 14 WebKit Commit Bot 2018-11-22 10:11:47 PST
Comment on attachment 355372 [details]
Patch

Clearing flags on attachment: 355372

Committed r238447: <https://trac.webkit.org/changeset/238447>
Comment 15 WebKit Commit Bot 2018-11-22 10:11:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-11-22 10:12:32 PST
<rdar://problem/46214651>
Comment 17 Derk-Jan Hartman 2018-12-10 02:37:36 PST
My thanks to everyone for fixing this issue so quickly. I found this bug 3 years ago already and meant to file it several times yet never managed to get around to it. Now I feel a tad guilty for not filing it sooner ;)