Bug 264897 – Text controls: Intrinsic widths should not be smaller than AverageCharWidth() * N

• Status: NEW
• Component: Text

[Ahmad Saleem]

Hi Team,

While going through Blink's commit and old bug, I came across another failing test case:

Test Case: https://jsfiddle.net/x6nfb0e4/show

Blink Commit: https://chromium.googlesource.com/chromium/src/+/a00094b9b640c10fff554af6dcb4a68d78300321

WebKit Source: https://searchfox.org/wubkat/source/Source/WebCore/platform/graphics/FontCascade.cpp#408

___

Didn't try but something like this?

{
    bool success = hasValidAverageCharWidth();
    if (success)
        const float width = primaryFont().avgCharWidth();
        width = std::max<float>(width, roundf(width)); // FIXME: primaryFont() might not correspond to firstFamily().
    return success;
}

___

Firefox Nightly 121 and Chrome Canary 121 both pass this test. I tried on WebKit ToT to confirm it is not font issue like access to Ahem usually have.

Thanks!

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #0)
> Hi Team,
> 
> While going through Blink's commit and old bug, I came across another
> failing test case:
> 
> Test Case: https://jsfiddle.net/x6nfb0e4/show
> 
> Blink Commit:
> https://chromium.googlesource.com/chromium/src/+/
> a00094b9b640c10fff554af6dcb4a68d78300321
> 
> WebKit Source:
> https://searchfox.org/wubkat/source/Source/WebCore/platform/graphics/
> FontCascade.cpp#408
> 
> ___
> 
> Didn't try but something like this?
> 
> {
>     bool success = hasValidAverageCharWidth();
>     if (success)
>         const float width = primaryFont().avgCharWidth();
>         width = std::max<float>(width, roundf(width)); // FIXME:
> primaryFont() might not correspond to firstFamily().
>     return success;
> }
> 
> ___
> 
> Firefox Nightly 121 and Chrome Canary 121 both pass this test. I tried on
> WebKit ToT to confirm it is not font issue like access to Ahem usually have.
> 
> Thanks!

This compiles and fixes the test case:

{
    bool success = hasValidAverageCharWidth();
    if (success)
        width = std::max<float>(primaryFont().avgCharWidth(), roundf(primaryFont().avgCharWidth())); // FIXME: primaryFont() might not correspond to firstFamily().
    return success;
}

[Radar WebKit Bug Importer]

<rdar://problem/118727006>

[Ahmad Saleem]

This test will fail in WebKit test infrastructure with 'Harness Error' due to 'setTextSubpixelPositioning' being undefined.

[Ahmad Saleem]

PR - https://github.com/WebKit/WebKit/pull/21963

[Ahmad Saleem]

I took the change into 'RenderTextControl.cpp':

https://searchfox.org/wubkat/rev/8856ae172a088c3c1dbfcdcf8ac8a2fb87625ddd/Source/WebCore/rendering/RenderTextControl.cpp#138

float RenderTextControl::getAverageCharWidth()
{
    // We apply roundf() only if the fractional part of 'width' is >= 0.5
    // because:
    // * We have done it for a long time.
    // * Removing roundf() would make the intrinsic width smaller, and it
    //   would have a compatibility risk.
    float width = style().fontCascade().primaryFont().avgCharWidth();
    if (style().fontCascade().fastAverageCharWidthIfAvailable(width))
        return std::max(width, roundf(width));

    const UChar ch = '0';
    const String str = span(ch);
    const FontCascade& font = style().fontCascade();
    TextRun textRun = constructTextRun(str, style(), ExpansionBehavior::allowRightOnly());
    return font.width(textRun);
}

&

float RenderTextControl::getAverageCharWidth()
{
    // We apply roundf() only if the fractional part of 'width' is >= 0.5
    // because:
    // * We have done it for a long time.
    // * Removing roundf() would make the intrinsic width smaller, and it
    //   would have a compatibility risk.
    float width;
    if (style().fontCascade().fastAverageCharWidthIfAvailable(width))
        return std::max(width, roundf(width));

    const UChar ch = '0';
    const String str = span(ch);
    const FontCascade& font = style().fontCascade();
    TextRun textRun = constructTextRun(str, style(), ExpansionBehavior::allowRightOnly());
    return font.width(textRun);
}

___

Both didn't fix this. :-(

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/38441