Bug 264897
| Summary: | Text controls: Intrinsic widths should not be smaller than AverageCharWidth() * N | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ahmad Saleem <ahmad.saleem792> |
| Component: | Text | Assignee: | Ahmad Saleem <ahmad.saleem792> |
| 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 | ||
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!
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
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