Bug 19161 - wrong font size when css font-family includes monospace
: wrong font size when css font-family includes monospace
Status: RESOLVED FIXED
: WebKit
CSS
: 525.x (Safari 3.1)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
: GoogleBug
:
: 24855
  Show dependency treegraph
 
Reported: 2008-05-20 16:24 PST by
Modified: 2009-08-13 16:37 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-05-20 16:24:30 PST
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 From 2008-05-20 16:24:52 PST -------
Created an attachment (id=21261) [details]
test case
------- Comment #2 From 2008-05-20 16:34:24 PST -------
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 From 2008-05-20 16:37:41 PST -------
Perhaps related to bug 13139.
------- Comment #4 From 2008-06-13 11:45:12 PST -------
Created an attachment (id=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 From 2008-07-24 10:37:54 PST -------
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 From 2009-04-13 18:27:17 PST -------
*** Bug 19643 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2009-04-14 15:35:33 PST -------
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 From 2009-04-20 17:03:46 PST -------
(In reply to comment #4)
> Created an attachment (id=21685) [details] [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 From 2009-04-22 12:47:26 PST -------
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 From 2009-04-22 14:13:59 PST -------
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 From 2009-04-22 14:19:06 PST -------
Test-case that show's the firefox behavior: http://tinyurl.com/dn89xe
------- Comment #12 From 2009-04-29 13:56:06 PST -------
Created an attachment (id=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 From 2009-04-30 18:30:33 PST -------
Textarea's also use the smaller font-size in IE and window's webkit (which uses monospace as the font-family).
------- Comment #14 From 2009-07-28 12:00:21 PST -------
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 From 2009-07-28 12:02:30 PST -------
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 From 2009-07-28 13:17:20 PST -------
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 From 2009-07-28 13:22:10 PST -------
(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 From 2009-08-05 12:46:47 PST -------
Created an attachment (id=34162) [details]
First (somewhat blind) stab at this.


---
 2 files changed, 18 insertions(+), 23 deletions(-)
------- Comment #19 From 2009-08-06 09:21:08 PST -------
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 From 2009-08-06 11:55:11 PST -------
Created an attachment (id=34215) [details]
Now a patch which works


---
 6 files changed, 64 insertions(+), 59 deletions(-)
------- Comment #21 From 2009-08-06 13:28:43 PST -------
Created an attachment (id=34224) [details]
Full fix with tests


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


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


---
 14 files changed, 226 insertions(+), 63 deletions(-)
------- Comment #25 From 2009-08-12 14:17:19 PST -------
(From update of attachment 34230 [details])
r=me
------- Comment #26 From 2009-08-12 14:31:59 PST -------
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 From 2009-08-12 17:10:16 PST -------
(From update of attachment 34230 [details])
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 From 2009-08-13 13:27:51 PST -------
(From update of attachment 34230 [details])
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 From 2009-08-13 13:56:54 PST -------
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 From 2009-08-13 16:37:05 PST -------
Test updates in r47250 and r47251