935056 - In SVG, small font-sizes are not honored: "minimum font size" limit is enforced

Status: RESOLVED FIXED

(Core :: SVG, defect, P4)

Description

[R. Shelton]

Attachment: font-size.svg

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025151332

Steps to reproduce:

Load the attached SVG file.  This has font-size declared in style classes, from 5px through 35px.


Actual results:

There appears to be a minimum px size acceptable for font-size, somewhat larger than 10px.  All sizes declared smaller than that are displayed in this minimum size.  If the image is scaled (by varying the size of the window), this minimum size is also scaled, so it's not an absolute font size that is the limiting factor.


Expected results:

Text declared with smaller font-sizes than the de facto minimum should be displayed smaller.  This used to work; it broke in the last week or so, probably in FF 25.

Comment 1

[KLB]

I don't fully understand the problem. I tested the attached SVG file in FF 24, 25 & 26 (On Win7). I also tested on Google Chrome.  The provided file behaved identically on all browsers.

Could you please provide some screen captures and a clearer explanation of what isn't working correctly?

Comment 2

[Robert Longson [:longsonr]]

I suspect you have a minimum font size set. SVG text should ignore that but no longer does.

Comment 3

[Masatoshi Kimura [:emk]]

Bug 839955 is very suspicious.
Reporter, do you see the problem if you flipped "svg.text.css-frames.enabled" from about:config?

Comment 4

[R. Shelton]

Yes, setting svg.text.css-frames.enabled to false makes the problem go away, and the file displays as should (and as it does in Chrome).  I don't understand what this feature does (are about:config items documented somewhere?), but this behavior of it is really wrong-headed.  In the test file (font-size.svg), the size of the smallest string is not all that small.  Something is *interpreting* it as small because "5px" seems small, but the actual size that translates into depends entirely on the coordinate system of the SVG file.  If someone wants a minimum text size, testing against the px value is the wrong way to do it -- the coord system in the test file leads to a "minimum font size" that is enormous.  Text in an SVG file is a graphical object, and should (at least by default) preserve its relationship to other objects; otherwise labels will overwhelm the graphics (and other text strings) -- and the typical user (me) will have no idea what's causing the problem.

Comment 5

[R. Shelton]

And yes, I do have a minimum text size set, to 12.  I had completely forgotten about that.  As I indicated above, the test file is being displayed with a minimum font size significantly larger than 12pt (the size I expected when I set the option) -- and as I argued above, enforcing any minimum size in SVG is probably the wrong answer.

Comment 6

[R. Shelton]

Another wrinkle:  I also have the option "Allow pages to set their own fonts" checked.  In ordinary text this seems to override my minimum size of 12pt.  It should also apply to SVG.

Comment 7

[R. Shelton]

I take back some of that last comment:  I can't be sure whether the minimum size is ignored in ordinary text.  The minimum shown in ordinary text is much smaller than 12pt, but there does seem to be a minimum.

Comment 8

[R. Shelton]

To confirm:

* setting svg.text.css-frames.enabled to false displays the file correctly.

* leaving svg.text.css-frames.enabled at true, but setting minimum font size to None in options also displays the file correctly, regardless of the "Allow pages to set fonts" option.

* leaving svg.text.css-frames.enabled at true, setting a minimum font size to X interprets X as a user unit value in SVG, not a pt value, and enforces this minimum regardless of the "Allow pages to set fonts" option.

Comment 9

[Cameron McCormack (:heycam)]

Thanks for confirming, R. Shelton.  This is definitely a regression then with the new SVG text layout support.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: patch

I *think* I'm safe in accessing aFont->mAllowZoom here when we're computing the font-size value, as indicated in the comment.  Also, if you want me to rename mAllowZoom / -x-text-zoom to something that also encompasses whether minimum font sizes apply, I can do that.

Comment 12

[[:Aleksej]]

Reproduced with 2013-11-13-03-02-05-mozilla-central-firefox-28.0a1.en-US.linux-x86_64.

Comment 13

[Cameron McCormack (:heycam)]

Probably not a serious enough bug to warrant landing on Release, but setting tracking? all the same.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 828364 [details] [diff] [review]
patch

r=dbaron.  (Though I'm not sure what the comment is suggesting might not be safe, but is safe in this case.)

Comment 15

[Cameron McCormack (:heycam)]

The comment is (trying to) say that aFont->mAllowZoom will always have a valid value, since if aRuleData->ValueForTextZoom()->GetUnit() == eCSSUnit_Null, we will have started with an inherited struct, and in all other cases we assign to mAllowZoom.

Comment 16

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d39a3544a287

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d39a3544a287

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Regression from 25, we can take this as an uplift to branches if low risk fix seeing as we're quite late in FF26 beta please nominate asap for landing this week.

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 828364 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 839955
User impact if declined: text in SVG documents shown at incorrect size if a minimum font size is set
Testing completed (on m-c, etc.): manual testing, plus an automated test; landed on m-c yesterday
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 20

[Cameron McCormack (:heycam)]

Backed out for maybe causing bug 939980:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed622e651e1e

It's an intermittent orange that triggers about four times a day, so we should know soon enough whether it did.  I'll re-land it, and on beta/aurora, if it turns out that it didn't cause the intermittent orange.

Comment 21

[Cameron McCormack (:heycam)]

The intermittent failures have stopped with the backout, so I won't land this on beta/aurora.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

We can wait for this to ride the trains, untracking.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cameron, have you got any idea what's going on here? We need to fix this regression somehow.

Comment 24

[Cameron McCormack (:heycam)]

I am not sure.  Landing a slight tweak of this patch that always initializes allowZoom, and uses it instead of font->mAllowZoom:

https://hg.mozilla.org/integration/mozilla-inbound/rev/29e6539961c3

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/437fc37b3ca7 for re-causing bug 939980, https://tbpl.mozilla.org/php/getParsedLog.php?id=31476773&tree=Mozilla-Inbound

Comment 26

[u279076]

Is status-firefox28:fixed accurate now that there's a backout?

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

nope, changing it back to affected.

Comment 28

[Cameron McCormack (:heycam)]

I remain stumped by the intermittent failures, but pushing a try run just now on inbound's tip I couldn't reproduce the failure with a bunch of retriggers.  So, relanding to see if it sticks again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3425d772b2ba

Comment 29

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=6efaf80f1400

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=34066411&tree=Mozilla-Inbound

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88ae89969aad

Comment 31

[Ryan VanderMeulen [:RyanVM]]

And no, that wasn't the only one.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this has been around now since FF25 without causing any obvious amounts of user pain it's not really looking like something we need to be tracking as a potential release-blocking bug but if there's a low risk fix ready in the next couple of weeks we can consider an uplift nomination to Aurora/Beta.

Comment 33

[Mike]

This worked fine for me in firefox whatever versionIhavebeforethe31.0update (30.0 I think)

In 31.0 the css font-size:12% stops being smaller somewhere around 50%. This page illustrates the problem.

<?xml version="1.0" encoding="utf-8" ?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en" >
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Development Results Server</title>
</head>
<body>
<div><span style="font-size:100%" >100%!</span></div>
<div><span style="font-size:90%" >90%!</span></div>
<div><span style="font-size:80%" >80%!</span></div>
<div><span style="font-size:70%" >70%!</span></div>
<div><span style="font-size:60%" >60%!</span></div>
<div><span style="font-size:50%" >50%?</span></div>
<div><span style="font-size:40%" >40%?</span></div>
<div><span style="font-size:30%" >30%?</span></div>
<div><span style="font-size:20%" >20%?</span></div>
</div>
</body></html>

Comment 34

[u279076]

(In reply to Mike from comment #33)
> This worked fine for me in firefox whatever versionIhavebeforethe31.0update
> (30.0 I think)

If this particular case is resolved for you by downgrading to Firefox 30 or earlier then you're likely experiencing a different bug. The bug described in this report has been around since Firefox 25.

Cameron, I'm assuming by the lack of activity here that this has fallen off your radar. Are you still working on this?

Comment 35

[Vincent Bernat]

This bug still exists in current versions of Firefox. Could the bug be updated to tell it affects current versions of Firefox?

Comment 36

[Robert Longson [:longsonr]]

If it's not in the state FIXED then that's sign enough that it affects current versions of Firefox. No updates necessary.

Comment 38

[Robert Longson [:longsonr]]

Maybe 7 years has improved things...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=035b7faed0dbc24e1b90a058fe184de5f052c8f4

Comment 39

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e69b39c2fdfa8733e0e0179c8779df655197b4f

Comment 40

[Robert Longson [:longsonr]]

Attachment: Bug 935056 - Don't apply minimum font sizes to SVG text. r=heycam r=dbaron

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eef199bbda9
Don't apply minimum font sizes to SVG text. r=heycam

Comment 42

[Razvan Maries]

Backed out for perma failures at 935056-1.html.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=82f9e4bad64592efb70ded65f86e566d3ca2c664&selectedTaskRun=XLKJSKgkQtiiC9UboMdlgQ-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301968735&repo=autoland&lineNumber=24637

Backout: https://hg.mozilla.org/integration/autoland/rev/a0bfb994a6db0feaaafb9beb9843f3822f0f2e6d

Comment 43

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee8953a65569c6f2bcb4c0236f67b4f251a856f&selectedTaskRun=Loxq6qzZRhaguhaouQ431Q-0

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afdf4d60166d
Don't apply minimum font sizes to SVG text. r=heycam

Comment 45

[Robert Longson [:longsonr]]

Looks like the fuzzy-if for android needs to be at least 324

test-pref(font.minimum-size.x-western,32) fuzzy-if(Android,0-45,0-324) == 935056-1.html 935056-1-ref.html

Comment 46

[Robert Longson [:longsonr]]

How do I fix that or do you back it out and I reland it with the new value?

Comment 47

[Noemi Erli[:noemi_erli]]

Backed out changeset afdf4d60166d (Bug 935056) for causing Android reftest failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=afdf4d60166d4a55dc811a85b9f2e5964fdb5a90&searchStr=android%2Creftest&selectedTaskRun=HYTvXMOCQ9O7E9fVeaz7Ew-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302064133&repo=autoland&lineNumber=11609

Backout: https://hg.mozilla.org/integration/autoland/rev/2a5b621c57c211dcb6792a441a3f806a99201244

Comment 48

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/afdf4d60166d

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/256602905ce8
Don't apply minimum font sizes to SVG text. r=heycam

Comment 50

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/256602905ce8