Bug 19161 - wrong font size when css font-family includes monospace
: wrong font size when css font-family includes monospace
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 525.x (Safari 3.1)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Eric Seidel
: GoogleBug
Depends on:
Blocks: 24855
  Show dependency treegraph
 
Reported: 2008-05-20 16:24 PDT by Eric Seidel
Modified: 2009-08-13 16:37 PDT (History)
10 users (show)

See Also:


Attachments
test case (508 bytes, text/html)
2008-05-20 16:24 PDT, Eric Seidel
no flags Details
screenshot comparing safari, firefox, ie (29.28 KB, image/png)
2008-06-13 11:45 PDT, Evan Martin
no flags Details
another test case : <pre> vs monospace, serif, sans-serif (419 bytes, text/html)
2009-04-29 13:56 PDT, Jungshik Shin
no flags Details
First (somewhat blind) stab at this. (7.42 KB, patch)
2009-08-05 12:46 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Now a patch which works (17.02 KB, patch)
2009-08-06 11:55 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Full fix with tests (20.29 KB, patch)
2009-08-06 13:28 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Add more info to the ChangeLog for easier review (21.00 KB, patch)
2009-08-06 13:42 PDT, Eric Seidel
no flags Details | Formatted Diff | Diff
Visual test showing the extra bug I had to fix (partial font-family fallback inheritance) (248 bytes, text/html)
2009-08-06 14:30 PDT, Eric Seidel
no flags Details
Oops, now includes missing test files. (26.72 KB, patch)
2009-08-06 14:33 PDT, Eric Seidel
hyatt: review+
eric: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2008-05-20 16:24:30 PDT
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
Comment 1 Eric Seidel 2008-05-20 16:24:52 PDT
Created attachment 21261 [details]
test case
Comment 2 mitz@webkit.org 2008-05-20 16:34:24 PDT
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).
Comment 3 mitz@webkit.org 2008-05-20 16:37:41 PDT
Perhaps related to bug 13139.
Comment 4 Evan Martin 2008-06-13 11:45:12 PDT
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.
Comment 5 Eric Seidel 2008-07-24 10:37:54 PDT
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.
Comment 6 Tony Chang 2009-04-13 18:27:17 PDT
*** Bug 19643 has been marked as a duplicate of this bug. ***
Comment 7 Tony Chang 2009-04-14 15:35:33 PDT
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').
Comment 8 Jungshik Shin 2009-04-20 17:03:46 PDT
(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. 



Comment 9 Dave Hyatt 2009-04-22 12:47:26 PDT
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.


Comment 10 Ojan Vafai 2009-04-22 14:13:59 PDT
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.
Comment 11 Ojan Vafai 2009-04-22 14:19:06 PDT
Test-case that show's the firefox behavior: http://tinyurl.com/dn89xe
Comment 12 Jungshik Shin 2009-04-29 13:56:06 PDT
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.
Comment 13 Ojan Vafai 2009-04-30 18:30:33 PDT
Textarea's also use the smaller font-size in IE and window's webkit (which uses monospace as the font-family).
Comment 14 Eric Seidel 2009-07-28 12:00:21 PDT
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.
Comment 15 Eric Seidel 2009-07-28 12:02:30 PDT
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.
Comment 16 Ojan Vafai 2009-07-28 13:17:20 PDT
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).
Comment 17 Eric Seidel 2009-07-28 13:22:10 PDT
(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.
Comment 18 Eric Seidel 2009-08-05 12:46:47 PDT
Created attachment 34162 [details]
First (somewhat blind) stab at this.


---
 2 files changed, 18 insertions(+), 23 deletions(-)
Comment 19 Eric Seidel 2009-08-06 09:21:08 PDT
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?
Comment 20 Eric Seidel 2009-08-06 11:55:11 PDT
Created attachment 34215 [details]
Now a patch which works


---
 6 files changed, 64 insertions(+), 59 deletions(-)
Comment 21 Eric Seidel 2009-08-06 13:28:43 PDT
Created attachment 34224 [details]
Full fix with tests


---
 8 files changed, 119 insertions(+), 63 deletions(-)
Comment 22 Eric Seidel 2009-08-06 13:42:04 PDT
Created attachment 34226 [details]
Add more info to the ChangeLog for easier review


---
 8 files changed, 130 insertions(+), 63 deletions(-)
Comment 23 Eric Seidel 2009-08-06 14:30:35 PDT
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. :)
Comment 24 Eric Seidel 2009-08-06 14:33:23 PDT
Created attachment 34230 [details]
Oops, now includes missing test files.


---
 14 files changed, 226 insertions(+), 63 deletions(-)
Comment 25 Dave Hyatt 2009-08-12 14:17:19 PDT
Comment on attachment 34230 [details]
Oops, now includes missing test files.

r=me
Comment 26 Eric Seidel 2009-08-12 14:31:59 PDT
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.
Comment 27 Eric Seidel 2009-08-12 17:10:16 PDT
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
Comment 28 Eric Seidel 2009-08-13 13:27:51 PDT
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
Comment 29 Eric Seidel 2009-08-13 13:56:54 PDT
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
Comment 30 Eric Seidel 2009-08-13 16:37:05 PDT
Test updates in r47250 and r47251