Bug 25566 – REGRESSION: text inputs with "size" are often smaller than before, but sometimes bigger (originally noticed in bugs.webkit.org text fields)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Eric Seidel (no email)]

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?

[Alexey Proskuryakov]

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).

[Alexey Proskuryakov]

(measured in number of characters, that is)

[Alexey Proskuryakov]

Textareas in particular are wider in IE in pixels, because they use a non-proportional font there.

[Ojan Vafai]

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.

[Mark Rowe (bdash)]

<rdar://problem/6885824>

[Simon Fraser (smfr)]

Created attachment 30302 [details]
Testcase

It seems wrong that <input size="60"> doesn't show 60 characters. Testcase attached.

[Simon Fraser (smfr)]

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."

[Ian 'Hixie' Hickson]

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

[John Sullivan]

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.

[Eric Seidel (no email)]

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.

[Ojan Vafai]

Created attachment 30527 [details]
Sample code for retreiving the avgCharWidth/maxCharWidth from the hhea/hmtx tables.

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 45923 [details]
Improve text control intrinsic widths.

[WebKit Review Bot]

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

[Ojan Vafai]

Comment on attachment 45923 [details]
Improve text control intrinsic widths.

Bugzilla-tool is causing me problems. This is ready for review.

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 45929 [details]
Improve text control intrinsic widths. Linter errors fixed.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45929 [details] without any errors.

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.

[Ojan Vafai]

Created attachment 46171 [details]
Improve text control intrinsic widths. Addressed mitz's comments.

[Ojan Vafai]

(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?

[Dave Hyatt]

Comment on attachment 46171 [details]
Improve text control intrinsic widths. Addressed mitz's comments.

r=me

[Eric Seidel (no email)]

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.

[Ojan Vafai]

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.

[Sam Weinig]

Comment on attachment 48664 [details]
Minor changes to compile on Tiger

Marking as a patch.

[Ojan Vafai]

Committed r54748: <http://trac.webkit.org/changeset/54748>