Bug 25566 - REGRESSION: text inputs with "size" are often smaller than before, but sometimes bigger (originally noticed in bugs.webkit.org text fields)
Summary: REGRESSION: text inputs with "size" are often smaller than before, but someti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Ojan Vafai
URL:
Keywords: InRadar, Regression
Depends on:
Blocks: 25958
  Show dependency treegraph
 
Reported: 2009-05-05 05:42 PDT by Eric Seidel (no email)
Modified: 2010-02-12 19:20 PST (History)
12 users (show)

See Also:


Attachments
Testcase (508 bytes, text/html)
2009-05-13 17:20 PDT, Simon Fraser (smfr)
no flags Details
Sample code for retreiving the avgCharWidth/maxCharWidth from the hhea/hmtx tables. (5.40 KB, patch)
2009-05-21 01:26 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Improve text control intrinsic widths. (15.00 KB, patch)
2010-01-05 12:58 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
New results for Leopard. Still need to get results for Tiger/Windows. (deleted)
2010-01-05 13:18 PST, Ojan Vafai
eric: review+
Details | Formatted Diff | Diff
Improve text control intrinsic widths. Linter errors fixed. (14.95 KB, patch)
2010-01-05 13:28 PST, Ojan Vafai
mitz: review-
Details | Formatted Diff | Diff
Improve text control intrinsic widths. Addressed mitz's comments. (13.25 KB, patch)
2010-01-08 16:04 PST, Ojan Vafai
hyatt: review+
Details | Formatted Diff | Diff
Minor changes to compile on Tiger (14.24 KB, patch)
2010-02-12 13:54 PST, Ojan Vafai
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-05-05 05:42:24 PDT
REGRESSION: bugs.webkit.org text fields are much smaller in tot

I'm not sure if this is good behavior or not.

I suspect this could be related to Ojan's recent avg-char width change.

Marking this as a regression for now, even though maybe it's a progression?
Comment 1 Alexey Proskuryakov 2009-05-05 06:00:20 PDT
Both text inputs and textareas are shorter than they used to be. It looks like we are closer to IE now (if anything, our text fields are still slightly wider).
Comment 2 Alexey Proskuryakov 2009-05-05 06:03:21 PDT
(measured in number of characters, that is)
Comment 3 Alexey Proskuryakov 2009-05-05 06:05:18 PDT
Textareas in particular are wider in IE in pixels, because they use a non-proportional font there.
Comment 4 Ojan Vafai 2009-05-05 21:11:32 PDT
I measured the width of the "URL" input as an example. It's an input size=60 with the default font. On Windows, we match IE exactly. On mac we're now considerably smaller, where we used to be considerably larger. There are a few issues at play here.

1. Our computation of maxCharWidth on the mac is often ~20px smaller than the windows value. I've filed bug 25581 for that issue.
2. Our computation of avgCharWidth is sometimes off from Windows.
3. On Mac we use floats for avgCharWidth/maxCharWidth, but the Windows APIs return ints.
4. We use Lucida Grande as the default font on Macs and MS Shell Dlg/Monospace as the default font on Windows. Those fonts have different metrics. That we got the same numbers before was coincidence as best I can tell.

To give context the way we compute widths of text controls is:
input width = avgCharWidth*size + (maxCharWidth - avgCharWidth)
textarea width = avgCharWidth * cols

Here are offsetWidths for <input size=60> and <textarea cols=60> with the default font (Lucida Grande on Mac, MS Shell Dlg / MonoSpace on Win):

Browser                     Input   Textarea
IE                                 386     503
WebKit nightly win   386     503
WebKit nightly mac  344     345
Safari 4 beta win      426     509
Safari 4 beta mac     426     441

For font-family: monospace:
Browser                     Input   Textarea
IE                                 487     503
WebKit nightly win   486     503
WebKit nightly mac  430     441
Safari 4 beta win      486     509
Safari 4 beta mac     428     443

For font-family: arial:
Browser                     Input   Textarea
IE                                 395     383
WebKit nightly win   395     383
WebKit nightly mac  315     311
Safari 4 beta win      426     449
Safari 4 beta mac     368     383

In terms of default fonts being different, I don't know what we can do about that. In terms of getting different values for the widths, there's a number of options. I'll let bug 25581 be for maxCharWidth and focus on avgCharWidth here.

To get the correct avgCharWidth, I tried
-getting the value out of the OS/2 table
-computing it according to http://developer.apple.com/textfonts/TTRefMan/RM06/Chap6OS2.html
-computing it according to http://www.microsoft.com/typography/OTSpec/os2.htm#acw
-doing all of the above and round/ceil/flooring the result

None of those gave numbers that exactly matched Windows metrics. The thing currently checked in implements the Apple style avgCharWidth (http://developer.apple.com/textfonts/TTRefMan/RM06/Chap6OS2.html).

So, some options moving forward:
1. Get the old metrics on the Mac by setting avgCharWidth=width('0'), maxCharWidth=0.
2. Same as above, but use the new maxCharWidth value that we compute.
3. Try and figure out exactly how Windows computes the avgCharWidth it returns (e.g. by generating fonts with known metrics and seeing what Windows returns).
4. Live with what we have now and just improve maxCharWidth (i.e. fix bug 25581), which would improve metrics for text inputs only.
5. Hard-code values for the 10+ fonts we care about and then fallback to one of the above methods.

I cringe at saying this, but I lean towards option 5. It's the only way we can match IE metrics here, short of doing option 3, which I'm skeptical can really succeed. There are cases where the old metrics are better and cases where the new metrics are better.
Comment 5 Mark Rowe (bdash) 2009-05-13 17:12:10 PDT
<rdar://problem/6885824>
Comment 6 Simon Fraser (smfr) 2009-05-13 17:20:29 PDT
Created attachment 30302 [details]
Testcase

It seems wrong that <input size="60"> doesn't show 60 characters. Testcase attached.
Comment 7 Simon Fraser (smfr) 2009-05-13 17:40:36 PDT
HTML5 spec isn't really precise on what "size" means; it just says "the user agent should ensure that at least that many characters are visible."
Comment 8 Ian 'Hixie' Hickson 2009-05-13 17:42:30 PDT
It's defined in detail in the rendering section:

http://www.whatwg.org/specs/web-apps/current-work/#the-input-element-as-a-text-entry-widget
Comment 9 John Sullivan 2009-05-14 16:22:30 PDT
This isn't as simple as all text fields getting thinner. On https://bugs.webkit.org/, three of the four text fields got thinner since Safari 4 Public Beta, but one of them (the bug number field in the footer) got wider.
Comment 10 Eric Seidel (no email) 2009-05-15 17:20:27 PDT
It's somewhat odd that we continue to mark this as a regression.  This was a very conscious behavior change, the intent was to match IE's size=/cols= behavior exactly (which has been done on windows).

The current decision on the table is what option that Ojan listed in comment #4 (https://bugs.webkit.org/show_bug.cgi?id=25566#c4) we want to pursue.
Comment 11 Ojan Vafai 2009-05-21 01:26:29 PDT
Created attachment 30527 [details]
Sample code for retreiving the avgCharWidth/maxCharWidth from the hhea/hmtx tables.
Comment 12 Ojan Vafai 2009-05-21 01:29:04 PDT
That patch isn't for review, it's just to show my final attempt at getting the same metrics as those Windows returns. This works well for some fonts (e.g. verdana, arial), but not others (e.g. courier new). 

It might be possible to iterate on this and get better results for the fonts that aren't good right now, but my hopes are not high. I lean towards just having a hard-coded table of font-name -> avgCharWidth, maxCharWidth.
Comment 13 Ojan Vafai 2009-05-22 02:44:09 PDT
I have a fix:
1. Windows uses a 13px default font for textareas and inputs. Mac uses 11px. col/size width calculations depend on 13px metrics. We need a quirk for this in RenderTextControl (i.e. pass 13 instead of 11 when the FontDescription is using the default font size instead of an explicit size).
2. Explicitly set font-sizes for textareas and inputs will match if we use rounded OS/2 avgCharWidth values.

Fonts that have a non-positive (invalid) OS/2 avgCharWidth value should use the width of '0' in order to match Safari 3. I have filed a follow-up bug 25958 to investigate matching IE in these cases. I have only seen these for Mac fonts (which are not an IE-compat issue).

Also, maxCharWidth still needs to be investigated. This is bug 25581. This is lower impact, however, as maxCharWidth has a fixed effect (~10px depending on the font), whereas avgCharWidth has an effect proportional to the size/cols value.

Sadly, I'm about to get on a plane to head off to vacation for one week. :( I will not have internet access. I intend to post a patch to fix this as described above on June 1st. If someone feels the urge to fix it before then, feel free.

I'll address bugs 25581 and 25958 once this bug is fixed.
Comment 14 Ojan Vafai 2010-01-05 12:53:57 PST
A bit delayed, but I'm finally back to addressing this. Patch coming soon. There hasn't been a Safari release with any of the text control width changes.
Comment 15 Ojan Vafai 2010-01-05 12:58:37 PST
Created attachment 45923 [details]
Improve text control intrinsic widths.
Comment 16 WebKit Review Bot 2010-01-05 12:59:34 PST
Attachment 45923 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderTextControlSingleLine.cpp:384:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/rendering/RenderTextControl.cpp:520:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/rendering/RenderTextControlMultiLine.cpp:85:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3
Comment 17 Ojan Vafai 2010-01-05 13:02:01 PST
Comment on attachment 45923 [details]
Improve text control intrinsic widths.

Bugzilla-tool is causing me problems. This is ready for review.
Comment 18 Ojan Vafai 2010-01-05 13:18:10 PST
Created attachment 45925 [details]
New results for Leopard. Still need to get results for Tiger/Windows.

All the interesting changes are in fast/forms/text-control-intrinsic-widths-expected.txt.
Comment 19 Ojan Vafai 2010-01-05 13:28:25 PST
Created attachment 45929 [details]
Improve text control intrinsic widths. Linter errors fixed.
Comment 20 WebKit Review Bot 2010-01-05 13:31:58 PST
style-queue ran check-webkit-style on attachment 45929 [details] without any errors.
Comment 21 mitz 2010-01-07 14:36:26 PST
Comment on attachment 45929 [details]
Improve text control intrinsic widths. Linter errors fixed.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 769fd6f..6fcc124 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog

The entire history and rationale for the change don’t really belong in the change log entry. The references to the bugs should suffice.

>      m_avgCharWidth = 0.f;
> +    m_maxCharWidth = 0.f;

The preferred style is "0", without the ".f".

> +    CFDataRef os2Table = CGFontCopyTableForTag(m_platformData.cgFont(), 'OS/2');
> +    if (os2Table) {

Since it’s only used in the scope of the if statement, you can define os2Table in the if statement itself:
       if (CFDataRef os2Table = CGFontCopyTableForTagm_platformData.cgFont(), 'OS/2'))

You are also failing release the CFDataRef. You should call CFRelease(os2Table) at the end of the block. Alternatively, you can use a RetainPtr<CFDataRef> like this:
       RetainPtr<CFDataRef> os2Table(AdoptCF, CGFontCopyTableFromTag(…

> +        const UInt8* os2 = CFDataGetBytePtr(os2Table);
> +        SInt16 os2AvgCharWidth = os2[2]*256 + os2[3];

You must check that os2Table is at least 4 bytes long before doing this!

> +    CFDataRef headTable = CGFontCopyTableForTag(m_platformData.cgFont(), 'head');
> +    if (headTable) {
> +        const UInt8* head = CFDataGetBytePtr(headTable);
> +        ushort uxMin = head[36]*256 + head[37];
> +        ushort uxMax = head[40]*256 + head[41];
> +        SInt16 xMin = static_cast<SInt16>(uxMin);
> +        SInt16 xMax = static_cast<SInt16>(uxMax);
> +        float diff = static_cast<float>(xMax - xMin);
> +        m_maxCharWidth = scaleEmToUnits(diff, m_unitsPerEm) * m_platformData.m_size;
> +    }

Same comments apply here.

> +    static HashMap<AtomicString, bool>* fontFamiliesWithInvalidCharWidthMap = 0;
> +    
> +    if (!fontFamiliesWithInvalidCharWidthMap) {
> +        fontFamiliesWithInvalidCharWidthMap = new HashMap<AtomicString, bool>;
> +        
> +        for (unsigned i = 0; i < sizeof(fontFamiliesWithInvalidCharWidth) / sizeof(fontFamiliesWithInvalidCharWidth[0]); i++)
> +            fontFamiliesWithInvalidCharWidthMap->set(AtomicString(fontFamiliesWithInvalidCharWidth[i]), true);
> +    }
> +
> +    return !fontFamiliesWithInvalidCharWidthMap->get(family);

It looks like you’re using a HashMap where everything maps to 'true'. You can use a HashSet!

r- based on the above issues, but ultimately I think Hyatt should weigh in on questions of substance.
Comment 22 Ojan Vafai 2010-01-08 16:04:06 PST
Created attachment 46171 [details]
Improve text control intrinsic widths. Addressed mitz's comments.
Comment 23 Ojan Vafai 2010-01-08 16:06:17 PST
(In reply to comment #21)
> The entire history and rationale for the change don’t really belong in the
> change log entry. The references to the bugs should suffice.

Fair enough. I was mostly trying to capture the list of sites that would be fixed, some of which are only in chromium internal bugs from before we launched.

> I think Hyatt should weigh in on questions of substance.

Agreed. Hyatt?
Comment 24 Dave Hyatt 2010-02-04 10:40:21 PST
Comment on attachment 46171 [details]
Improve text control intrinsic widths. Addressed mitz's comments.

r=me
Comment 25 Eric Seidel (no email) 2010-02-04 15:57:39 PST
Comment on attachment 45925 [details]
New results for Leopard. Still need to get results for Tiger/Windows.

Since hyatt r+'d the actual code change, rs=me on the layout test updates.
Comment 26 Ojan Vafai 2010-02-12 13:54:12 PST
Created attachment 48664 [details]
Minor changes to compile on Tiger

Only changes from patch 46171 are to add the copyFontTableForTag method (CGFontCopyTableForTag doesn't exist on Tiger) and to move the implementation of scaleEmToUnits from RenderTextControl.h to RenderTextControl.cpp. Sorry to add another iteration to this review.
Comment 27 Sam Weinig 2010-02-12 14:10:27 PST
Comment on attachment 48664 [details]
Minor changes to compile on Tiger

Marking as a patch.
Comment 28 Ojan Vafai 2010-02-12 19:20:36 PST
Committed r54748: <http://trac.webkit.org/changeset/54748>