Bug 264897

Summary:	Text controls: Intrinsic widths should not be smaller than AverageCharWidth() * N
Product:	WebKit	Reporter:	Ahmad Saleem <ahmad.saleem792>
Component:	Text	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	fantasai.bugs, mmaxfield, vitor.roriz, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	Safari Technology Preview
Hardware:	Unspecified
OS:	Unspecified

Description Ahmad Saleem 2023-11-15 13:58:55 PST

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!

Comment 1 Ahmad Saleem 2023-11-15 14:30:04 PST

(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;
}

Comment 2 Radar WebKit Bug Importer 2023-11-22 13:59:13 PST

<rdar://problem/118727006>

Comment 3 Ahmad Saleem 2023-11-22 15:49:40 PST

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

Comment 4 Ahmad Saleem 2023-12-18 18:11:46 PST

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

Comment 5 Ahmad Saleem 2024-04-12 14:06:47 PDT

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. :-(