Bug 68090 - SVG as image uses very tiny default font-size
Summary: SVG as image uses very tiny default font-size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 52892 93770 100269 103691 svgbgspecificity (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-14 10:47 PDT by Daniel Holbert
Modified: 2015-05-21 13:35 PDT (History)
22 users (show)

See Also:


Attachments
testcase (447 bytes, text/html)
2011-09-14 10:47 PDT, Daniel Holbert
no flags Details
Patch (11.37 KB, patch)
2011-12-14 14:12 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (7.85 KB, patch)
2015-05-15 16:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (6.25 KB, patch)
2015-05-15 17:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (991.46 KB, application/zip)
2015-05-15 17:45 PDT, Build Bot
no flags Details
Patch (6.74 KB, patch)
2015-05-21 09:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Holbert 2011-09-14 10:47:49 PDT
Created attachment 107354 [details]
testcase

STR: Load attached testcase

EXPECTED RESULTS: Text is same size (and readible) in both images.
ACTUAL RESULTS: Text is unreadable in the top image.

The only difference between the two images is that I explicitly specify font-size='16' in the lower one.
In Firefox & Opera, this is the default size, so it doesn't make a difference in the rendering.  In Chromium 14, though, the default size seems to be teeny tiny and unreadable.  This is <img>-specific -- if you edit the testcase to use <embed> or <iframe> instead of <img>, then the font becomes reasonably-sized.

Chromium version: 14.0.835.126 (Developer Build 99097 Linux) Ubuntu 11.10
Comment 1 Florin Malita 2011-12-14 14:12:14 PST
Created attachment 119294 [details]
Patch
Comment 2 Florin Malita 2011-12-14 14:13:16 PST
The problem is that SVGImage creates an embedded Page and indirectly a fresh/default-initialized Settings object. Hence, defaultFontSize is 0 for embedded text elements.

The patch above is a fist attempt at addressing the issue by inheriting relevant values from the current settings. There're a couple of things I'm unsure about though:

1) The plumbing for making the current Settings reference available in SVGImage::dataChanged() seems awkward, if even safe. Is there a better way to gain access to the current settings in that method?

2) I couldn't find a way to implement the test programmatically and ended up with a text pixel test - which I presume will have some variance across platforms. Is this avoidable?
Comment 3 mitz 2011-12-14 14:17:17 PST
Cached resources may be shared between documents that have different settings. It seems very wrong to arbitrarily let the first document requesting the image to be the one determining its default font size.
Comment 4 mitz 2011-12-14 14:18:25 PST
See also bug 20846.
Comment 5 Nikolas Zimmermann 2011-12-15 00:45:59 PST
Comment on attachment 119294 [details]
Patch

This test should move to LayoutTests/svg/as-image, I think its more suitable there, as it's really SVGImage-specific.
I'm not too familiar with the design of Settings & co, and thus would like to hear another opinion, before touching r? state.
Great that you discovered the root of this problem!
Comment 6 Nikolas Zimmermann 2011-12-15 00:52:40 PST
(In reply to comment #3)
> Cached resources may be shared between documents that have different settings. It seems very wrong to arbitrarily let the first document requesting the image to be the one determining its default font size.

That's true. But the common case is that a resources is shared between documents with the same settings (in eg. a regular browsing session).

Fixing that properly would involve marking CachedResources as "unique", to avoid to share it with another document. I worked on this during the early SVGImage-size-negotiation patches, and even had a patch for this. Though it's complex to get it right. I only implemented a simple logic, you could mark one resource as unique, and then the next time that resource was requested, it had to be reloaded from the wire. This is bad.

A correct design would allow us to, check whether a CachedResource should be unique or not, but at the point when the same resource is requested a 2nd time. Using the type of resources, say we know it's a SVGImage, we can then determine whether its okay to share these resources, or not.
That still wouldn't handle dynamic changes, eg. by changing the default font size from Safari. That would involve checking all resources, eg:

If two documents a.html / b.html share foo.svg, with the same min. font-size of 16, and you'd change a.htmls font-size to 14, then a new CachedResource would need to appear, that corresponds to a SVGImage with the different Settings object. (It's not necessary to actually create two CachedResources, one could invent another cache of SVGImage-by-Settings, inside of it, similar to how its done with SVGImageCache).

Anyhow, I'm just brainstorming, and wanted to note that the "real issue" is hard to fix.
This "workaround" will catch most common cases though, and fixes a real problem, that now hits us badly.
Comment 7 mitz 2011-12-15 08:12:12 PST
It's not obvious to my that *any* document's settings should affect the rendering of SVG images, nor that the settings that apply to them should be customizable. A global, hard-wired Settings object (say, initialized with WebKit's default defaults) could provide settings for all SVG images.
Comment 8 Florin Malita 2011-12-15 09:24:33 PST
(In reply to comments #3, #7)
> Cached resources may be shared between documents that have different settings. 

I see. Is this a concern in practice through? I'm fairly new to WK and not familiar with the area - can you describe what it would take to end up in a situation where resource-sharing documents have different settings?


> It seems very wrong to arbitrarily let the first document requesting the image to be the one determining its default font size.

Yes - in light of the above it does. But, pragmatically speaking, I'm with Nikolas: currently all cases are broken; if this workaround fixes the common use case, then it may be an acceptable temporary band-aid (for 20846 too) while a proper solution is implemented.


> It's not obvious to my that *any* document's settings should affect the rendering of SVG images, nor that the settings that apply to them should be customizable.

I agree it's not made obvious in the spec, but section 6.1 does reference CSS2 for the definition of shared attributes (IIRC that covers attribute resolution all the way to UA settings). And it seems the right thing to do (keeping HTML & SVG text elements consistent).

I've verified the behavior in FF & Opera: they both honor user font settings when rendering image-embedded SVGs. Furthermore, updated settings are reflected immediately.
Comment 9 Alexey Proskuryakov 2011-12-15 13:13:24 PST
I agree with Mitz - even an image SVG is still a separate document, and having embedding documents affect embedded ones is generally not advisable.

> A global, hard-wired Settings object (say, initialized with WebKit's default defaults) could provide settings for all SVG images.

Indeed.
Comment 10 Eric Seidel (no email) 2011-12-19 13:55:22 PST
Comment on attachment 119294 [details]
Patch

Really?  This seems strange.  SVG when used as an <img> should just be like a .gif used as an img.. We don't change the font-size (well, can't) for gif. :)  pdf is another good example.. even though Chromium doesn't support pdf-as-image (but Safari did... and may still?)
Comment 11 Eric Seidel (no email) 2011-12-19 13:56:33 PST
If authors want this behavior, tey should just embed the <svg> code directly in the HTML.  That would use CSS interitance and HTML5 parsing rules.  It's not clear that SVG-as-image makes sense for the web platform in general.
Comment 12 Eric Seidel (no email) 2011-12-19 13:56:56 PST
(full disclosure, I wrote WebKit's SVGImage support, and associated hacks.)
Comment 13 Nikolas Zimmermann 2011-12-20 00:22:02 PST
(In reply to comment #9)
> 
> I've verified the behavior in FF & Opera: they both honor user font settings when rendering image-embedded SVGs. Furthermore, updated settings are reflected immediately.

Alexey/Dan:
The SVG intrinsic size is already dependent on the host document - as explicitly specified in SVG and CSS 2.1. I fully agree, that its awkward that the font-size of an embedded SVG is dependent on the host document, but this is how its implicitly specified, and how it works in Opera/FF.

Of course, it's much easier for us, if we'd hard-code the default settings for the embedded SVG - I'm also not aware of any real world issue this would introduce, except compatibility.

If we want to ignore this specifically, we should get folks from Opera/FF on board, and fix it in all our implementations.

I'd propose we ignore the dynamic part of the font-size changes for now, and hardcode the default settings for SVGImage, for now.
Comment 14 Alexey Proskuryakov 2011-12-20 18:47:55 PST
I'm not sure if we're on the same page here.

Hardcoding settings doesn't seem to be an option anyone suggests. What I'm thinking of is using browser defaults for font size, not whatever size embedding document's settings object has. In practice, per-document settings normally have the same sizes as browser defaults say.
Comment 15 Tim Horton 2011-12-20 18:53:20 PST
(In reply to comment #14)
> I'm not sure if we're on the same page here.
> 
> Hardcoding settings doesn't seem to be an option anyone suggests. What I'm thinking of is using browser defaults for font size, not whatever size embedding document's settings object has. In practice, per-document settings normally have the same sizes as browser defaults say.

Using the browser defaults, as Dan and Alexey suggest, is exactly what happens in Firefox and Opera. SVG-in-<img> does *not* inherit text size from the parent element, it uses the default font-size that a fresh document would have. Inheriting there would make no sense, and would not match any browser.

My vote is definitely for using the defaults, if only for consistency's sake.
Comment 16 Nikolas Zimmermann 2011-12-21 00:24:41 PST
(In reply to comment #14)
> I'm not sure if we're on the same page here.
Oh we are I meant to say "hard-wired", not "hard-coded", agreeing to Dans comment "A global, hard-wired Settings object (say, initialized with WebKit's default defaults) could provide settings for all SVG images."
> 
> Hardcoding settings doesn't seem to be an option anyone suggests. What I'm thinking of is using browser defaults for font size, not whatever size embedding document's settings object has. In practice, per-document settings normally have the same sizes as browser defaults say.
Right.
Comment 17 Alexey Proskuryakov 2012-01-12 10:14:03 PST
See also: bug 76162.
Comment 18 Alexey Proskuryakov 2012-08-13 09:47:07 PDT
*** Bug 93770 has been marked as a duplicate of this bug. ***
Comment 19 Florin Malita 2012-11-30 05:22:57 PST
*** Bug 103691 has been marked as a duplicate of this bug. ***
Comment 20 Philip Rogers 2014-03-13 19:50:48 PDT
*** Bug 125788 has been marked as a duplicate of this bug. ***
Comment 21 Tamas Gergely 2014-03-27 08:40:00 PDT
(In reply to comment #20)
> *** Bug 125788 has been marked as a duplicate of this bug. ***

Is it right to assume that the default settings (default font size, default encoding, etc.) are absolutely global in a browser, and cannot be set as per page, tab or window? If yes, then changing these attributes to global (static in the class Settings) could solve the problem, however raises another one: on changing the defaults (from anywhere), all docs should be notified and rendered again. I'm not sure whether is should be an automatic, or should be initiated by the one who changed the defaults.
Comment 22 Alexey Proskuryakov 2014-03-27 09:31:09 PDT
> Is it right to assume that the default settings (default font size, default encoding, etc.) are absolutely global in a browser, and cannot be set as per page, tab or window?

No, settings are not global. For example, when a web view is used for part of browser UI, it's not affected by user specified settings for web content.
Comment 23 mitz 2015-04-28 19:26:57 PDT
<rdar://problem/12563956>
Comment 24 Alexey Proskuryakov 2015-05-15 13:02:59 PDT
*** Bug 52892 has been marked as a duplicate of this bug. ***
Comment 25 Said Abou-Hallawa 2015-05-15 16:12:43 PDT
Created attachment 253236 [details]
Patch
Comment 26 Said Abou-Hallawa 2015-05-15 17:08:02 PDT
Created attachment 253242 [details]
Patch
Comment 27 Said Abou-Hallawa 2015-05-15 17:14:58 PDT
This is the summery of the solutions which were discussed to fix this bug:

1) Tie the page which is created for the non-interactive SVG with the preferences so when they change, the resource page gets laid out:
  -- no infrastructure exists to make calls from WebCore to WebKit
  ++ being compatible with FireFox
2) Inherit the global settings from the top-level document:
  ++ The patch which Florin Malita uploaded does exactly that.
  ++ If preferences do not change while the SVG is in the cache, the SVG text with default font size will be rendered the same as the other text.
  -- The layout of the SVG will depend on when it is cached. Once it is cached it will not be notified if the preferences change.
  -- Different from FireFox if the preferences change.
3) Have a separate preference key for the non-interactive SVG default font size
  -- Safari removed its default font size from the preferences dialog so it does not make sense to add a new one just for the non-interactive SVG default font size.
  -- Different from FireFox.
4) After creating the page for the non-interactive resource, explicitly set the defaultFontSize to 16
  ++ The SVG text with default font size will be rendered the same regardless of the preferences when the SVG is firstly loaded.
  -- All the text with default font size in the page will change if the preferences change but the text in the non-interactive SVG will not.
  -- Different from FireFox.
5) Change initial value of Settings.m_defaultFontSize to be 16 instead of zero
  ++ Same as (4) but a little bit cleaner.

I uploaded two patches for solutions (4) and (5).
Comment 28 Build Bot 2015-05-15 17:45:23 PDT
Comment on attachment 253242 [details]
Patch

Attachment 253242 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5980583440154624

New failing tests:
svg/text/text-default-font-size.html
Comment 29 Build Bot 2015-05-15 17:45:29 PDT
Created attachment 253249 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 30 Darin Adler 2015-05-17 09:58:53 PDT
Comment on attachment 253242 [details]
Patch

This change looks fine. r=me

I also think we should be doing this for a lot more settings, not just defaultFontSize. For example, defaultFixedFontSize. Basically the default defaults in Settings should match the default defaults in WebPreferences unless there is a good reason they should *not* match. Someone should follow up and do that because I think we could have similar issues with various other settings too.

I also think that in the future SVG image rendering might want to respect *some* settings from the host page. Not sure which ones at this time, though.
Comment 31 Said Abou-Hallawa 2015-05-21 09:04:22 PDT
Created attachment 253528 [details]
Patch
Comment 32 WebKit Commit Bot 2015-05-21 10:36:42 PDT
Comment on attachment 253528 [details]
Patch

Clearing flags on attachment: 253528

Committed r184719: <http://trac.webkit.org/changeset/184719>
Comment 33 WebKit Commit Bot 2015-05-21 10:36:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Said Abou-Hallawa 2015-05-21 11:08:57 PDT
*** Bug 100269 has been marked as a duplicate of this bug. ***
Comment 35 Said Abou-Hallawa 2015-05-21 13:35:11 PDT
*** Bug 100269 has been marked as a duplicate of this bug. ***