Bug 19161 – wrong font size when css font-family includes monospace

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Seidel (no email)]

getComputedStyle(element, null).fontSize is wrong

This affects an internal Google application.

See test case.  TOT results:
foo bar baz
monospace: 13px
courier new: 16px
courier new,monospace: 13px

FF results are:
foo bar baz
monospace: 13px
courier new: 16px
courier new,monospace: 16px

[Eric Seidel (no email)]

Created attachment 21261 [details]
test case

The reported sizes are in fact the ones used. That "'courier new', monospace" uses 13px is probably a bug (I think there is a separate bug about it in Bugzilla).

Perhaps related to bug 13139.

[Evan Martin]

Created attachment 21685 [details]
screenshot comparing safari, firefox, ie

Screenshot showing Safari with a tiny default monospace font where FF and IE agree on a different one.

[Eric Seidel (no email)]

This seems to be a common complaint (to google) from Safari users using Google Reader or Google Groups, both of which use monospace fonts to display certain contents, and hit this bug.

[Tony Chang]

*** Bug 19643 has been marked as a duplicate of this bug. ***

[Tony Chang]

some more test cases at: http://ponderer.org/tests/monospace.html

In particular, having the font-family be "serif, monospace" or "times, monospace" causes the font to be sized to the default fixed width font size.  This is because the generic font family gets set in CSSStyleSelector.cpp:applyProperty() in the font order from left to right and the default font size is then determined based on the generic font family.

Firefox has a similar bug where it only uses the default fixed width font size if the font-family is monospace (no fallbacks).  So in Firefox, "foobar, monospace" uses the default variable width font size even though foobar doesn't exist.
  https://bugzilla.mozilla.org/show_bug.cgi?id=175415

IE doesn't seem to have any of these problems because there is only one default font size which I can't find the setting for.

The Firefox and WebKit bugs seems to exist because we can't take the time to look up font existence at style parse time.  If we have to maintain that constraint, I think Firefox's behavior is a bit better (only use the fixed font size when the font-family is only 'monospace').

[Jungshik Shin]

(In reply to comment #4)
> Created an attachment (id=21685) [review]
> screenshot comparing safari, firefox, ie
> 
> Screenshot showing Safari with a tiny default monospace font where FF and IE
> agree on a different one.

In this particular case, the style specified by Google Groups is :

<font face="Courier, Monospaced" class="fixed_width">

Class fixed_width gets these two from 
http://groups.google.com/groups/style.css

.fixed_width { font-family: fixed-width, monospace; font-size: 90%}
.msg .mb .fixed_width { font-family: fixed-width, monospace; font-size: 100%}

The computed font size in Safari 4beta is 10px while in Firefox3 is 12.08px. 

So, at least for Google Groups, there appears to be a separate issue that needs to be fixed. 

[Dave Hyatt]

This behavior is deliberate.  The assumption is that because the generic monospace family was listed that the monospace size should be used.  After all the font in front is in fact a monospace font.  It's not a bug in the sense that it was a deliberate design decision on my part.  Might be worth revisiting if it's causing incompatibilities though.

[Ojan Vafai]

Talked to hyatt on IRC. We came to the conclusion that we should match FF behavior to maximize compatibility. To be clear, this means we only use the fixed-width font-size if the font-family is set to *exactly* monospace. For font-family:'monospace', font-family:foo,monospace and font-family:monospace,foo, we should use the standard font-size.

Longer-term we should test a webkit build that has one font-size for all fonts and see how bad the compat hit is. If it's minor, then we can consider getting rid of the font preferences entirely for Safari/Chrome. According to Hyatt, noone especially wants these preferences. They are just leftovers from Netscape long ago that everyone matched for compatibility.

[Ojan Vafai]

Test-case that show's the firefox behavior: http://tinyurl.com/dn89xe

[Jungshik Shin]

Created attachment 29893 [details]
another test case : <pre> vs monospace, serif, sans-serif


There's another twist to this issue (below is rehash of a part of what I wrote at
http://code.google.com/p/chromium/issues/detail?id=10524#c8 with what Ojan replied. I'm not including what I heard from Frank Tang about two font size preferences.).

This test file has 4 paragraphs. The 1st is <pre> while the 2nd,3rd and 4th are divs styled with 'monospace', 'serif' and 'sans-serif'.

In IE8, the first is rendered in a smaller font than the other three. This is because IE8 (IE 6/7, too) apparently has a UA style for <pre> with 'font-size: small' (or something like that). 

In Firefox, if the encoding is set to one of CJK encodings, all four are rendered at the same size. If not, the first two paras are smaller than the 3rd and the 4th. This is because 

1) Firefox uses the same font size for <pre> and 'font-family: monospace' 
2) For CJK langgroups, the monospace font size is identical to that of proportional font size (other 4 CSS generic families).
For most other langgroups, the size for CSS 'monospace' generic is smaller than that for other generic families. 

In Safari, the behavior is the same as Firefox with the encoding set to one of non-CJK encodings (e.g. ISO-8859-1). BTW, I can make a single test file showing the impact of changing the encoding in Firefox. 

After making a change to make webkit behave identically to Firefox (i.e. only 'font-family: monospace' will use the font size for fixed-width), we may consider making webkit behave similarly to IE7/8. (Ojan's #3 at http://code.google.com/p/chromium/issues/detail?id=10524#c10 ) with the compat impact evaluated.

And, then we can also think about replacing the font size UI with the page zoom level UI. The font size pref. is (mainly) for accessibility needs, but just setting the font size larger is likely to break the page layout. Now that webkit has page-zoom implemented, it makes more sense to have a page zoom-level UI.

[Ojan Vafai]

Textarea's also use the smaller font-size in IE and window's webkit (which uses monospace as the font-family).

[Eric Seidel (no email)]

I believe this is the cause of https://bugs.webkit.org/show_bug.cgi?id=24855, and bug 24855 can be duped to this one.

[Eric Seidel (no email)]

Well, to be more clear: bug 24855 is caused by a bug in gmail/TrogEdit's workaround for bug 21033.   But this strangeness is what's causing their workaround to break.

Basically:
courier new: 13px
courier new, monspace: 12px

their code doesn't expect to ever see 12px, and fails.

[Ojan Vafai]

I think the issue in bug 24855 is that the font-size is actually smaller and can't be made the same size as the text around it by making the font-size value be "2". Even if the Gmail code added a workaround for the 12px case, all that would do is make the "normal" font-size be selected in the font menu. Which is an improvement, but not really the main issue in bug 24855.

That said, I agree with Eric. Bug 24855 would also be fixed with the proposed fix of matching FF behavior of only using the smaller font-size if the font-family is exactly the string monospace (no quotes).

[Eric Seidel (no email)]

(In reply to comment #16)
> I think the issue in bug 24855 is that the font-size is actually smaller and
> can't be made the same size as the text around it by making the font-size value
> be "2". Even if the Gmail code added a workaround for the 12px case, all that
> would do is make the "normal" font-size be selected in the font menu. Which is
> an improvement, but not really the main issue in bug 24855.
> 
> That said, I agree with Eric. Bug 24855 would also be fixed with the proposed
> fix of matching FF behavior of only using the smaller font-size if the
> font-family is exactly the string monospace (no quotes).

Oh.  I misunderstood bug 24855 then.  I thought that you could not change the font size at all once it was made courier new.  I now find that you can, it just never is the same size as the font around it.  In which case then yes, the rendering bug is best fixed by bug 19161.

[Eric Seidel (no email)]

Created attachment 34162 [details]
First (somewhat blind) stab at this.


---
 2 files changed, 18 insertions(+), 23 deletions(-)

[Eric Seidel (no email)]

It turns out that our current font fallback behavior is:

<div style="font-family: foo, bar">
<div style="font-family: monospace"></div>
</div>

The font-family list carried on the inner div is "monospace, bar", which is not what I would have expected.  I would have expected just "monospace" or "monospace, foo, bar".  But maybe that's correct behavior?

You can see this behavior implemented in the for loop inside CSSStyleSelector::applyProperty() case CSSPropertyFontFamily.

Why this matters is that this breaks my new "useFixedDefaultSize" logic.
    bool FontDescription::useFixedDefaultSize() const { return genericFamily() == MonospaceFamily && !family().next() && family().family() == "-webkit-monospace"; }
I had assumed the fallback list for the inner div would only have included "monospace".

Hyatt?  Could you tell me if our family list behavior is correct here?

[Eric Seidel (no email)]

Created attachment 34215 [details]
Now a patch which works


---
 6 files changed, 64 insertions(+), 59 deletions(-)

[Eric Seidel (no email)]

Created attachment 34224 [details]
Full fix with tests


---
 8 files changed, 119 insertions(+), 63 deletions(-)

[Eric Seidel (no email)]

Created attachment 34226 [details]
Add more info to the ChangeLog for easier review


---
 8 files changed, 130 insertions(+), 63 deletions(-)

[Eric Seidel (no email)]

Created attachment 34229 [details]
Visual test showing the extra bug I had to fix (partial font-family fallback inheritance)

I'm attaching this visual version of the test to the bug.  The attached patch has a dumpAsText version which tests the same thing w/o needing a pixel test.  This version is here to make reviewers lives easier. :)

[Eric Seidel (no email)]

Created attachment 34230 [details]
Oops, now includes missing test files.


---
 14 files changed, 226 insertions(+), 63 deletions(-)

[Dave Hyatt]

Comment on attachment 34230 [details]
Oops, now includes missing test files.

r=me

[Eric Seidel (no email)]

Hyatt and I had a long (informative) discussion over IRC.  I'm happy to provide the log to anyone interested.

Hyatt's concerns were that our previous behavior was self-consistent and that our new behavior (matching FF) is bizarre.

My opinion was that our previous behavior is a good thing if you start with the premise that having two separate default font sizes is a good thing.  FF's behavior "limits the scope" of the separate fixed-width default size to only affect font-family: monospace;  Likely they did this to more closely match IE, but I'm not sure.

IE only has one default font size.  FF and WK have separate sizes for "standard" fonts and "fixed width" fonts.  After this change the "fixed width" preference only affects text which is explicitly font-family: monospace.

[Eric Seidel (no email)]

Comment on attachment 34230 [details]
Oops, now includes missing test files.

Rejecting patch 34230 from commit-queue.  This patch will require manual commit.

W e b K i t T o o l s / S c r i p t s / b u i l d - w e b k i t failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 34230 [details]
Oops, now includes missing test files.

Rejecting patch 34230 from commit-queue.  This patch will require manual commit.

W e b K i t T o o l s / S c r i p t s / b u i l d - w e b k i t failed with exit code 1

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-expected.txt
	A	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-monospace-expected.txt
	A	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-monospace.html
	A	LayoutTests/fast/css/getComputedStyle/font-family-fallback-reset-expected.txt
	A	LayoutTests/fast/css/getComputedStyle/font-family-fallback-reset.html
	A	LayoutTests/fast/css/getComputedStyle/resources/computed-style-font-family-monospace.js
	A	LayoutTests/fast/css/getComputedStyle/resources/font-family-fallback-reset.js
	M	LayoutTests/platform/mac/css1/font_properties/font_family-expected.txt
	M	LayoutTests/platform/mac/css2.1/t1503-c522-font-family-00-b-expected.txt
	M	LayoutTests/platform/mac/fast/text/monospace-width-cache-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/platform/graphics/FontDescription.h
Committed r47229
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/modules/committers.py
r47228 = 2aa5a865f28f3a686ebb3c7a675d9c7414e6fcd9 (trunk)
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/FontDescription.h
	M	WebCore/css/CSSStyleSelector.cpp
	M	LayoutTests/platform/mac/fast/text/monospace-width-cache-expected.txt
	M	LayoutTests/platform/mac/css2.1/t1503-c522-font-family-00-b-expected.txt
	M	LayoutTests/platform/mac/css1/font_properties/font_family-expected.txt
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-monospace.html
	A	LayoutTests/fast/css/getComputedStyle/font-family-fallback-reset-expected.txt
	A	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-monospace-expected.txt
	A	LayoutTests/fast/css/getComputedStyle/font-family-fallback-reset.html
	A	LayoutTests/fast/css/getComputedStyle/resources/font-family-fallback-reset.js
	A	LayoutTests/fast/css/getComputedStyle/resources/computed-style-font-family-monospace.js
	M	LayoutTests/fast/css/getComputedStyle/computed-style-font-family-expected.txt
r47229 = 53ed0bc9178698557ffef09fec2e1dd1ac125daf (trunk)
First, rewinding head to replay your work on top of it...
Nothing to do.
http://trac.webkit.org/changeset/47229

[Eric Seidel (no email)]

Test updates in r47250 and r47251