Bug 233070

Summary: [Cocoa] REGRESSION(r281291): Text Style fonts don't have the correct weight set
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, heycam, kondapallykalyan, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Needs test
none
Needs test
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch heycam: review+

Description Myles C. Maxfield 2021-11-12 13:30:56 PST 
[Cocoa] Text style fonts shouldn't have their weight/width/slope overridden by font-weight/font-stretch/font-style
Comment 1 Myles C. Maxfield 2021-11-12 13:37:02 PST 
Created attachment 444106 [details]
Needs test
Comment 2 Myles C. Maxfield 2021-11-12 13:37:52 PST 
<rdar://problem/84663666>
Comment 3 Myles C. Maxfield 2021-11-12 13:39:27 PST 
Created attachment 444107 [details]
Needs test
Comment 4 Myles C. Maxfield 2021-11-12 14:35:14 PST 
Created attachment 444110 [details]
Patch
Comment 5 Myles C. Maxfield 2021-11-18 23:48:39 PST 
Created attachment 444784 [details]
Patch
Comment 6 Myles C. Maxfield 2021-11-19 15:09:14 PST 
Created attachment 444860 [details]
Patch
Comment 7 Myles C. Maxfield 2021-11-20 14:03:06 PST 
This needs more investigation, I think. We do:

textStyle = kCTUIFontTextStyleBody;
fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(textStyle, contentSizeCategory(), nullptr));
...
auto font = adoptCF(CTFontCreateWithFontDescriptor(fontDescriptor.get(), 0, nullptr));
...
fontDescription.setWeight(cssWeightOfSystemFont(font.get()));

So we’re creating the text style font, pulling out its weight, then setting the font-weight property to that, which should work, if we then re-apply that same weight later.

I think I need more investigation here. This patch might be wrong.
Comment 8 Myles C. Maxfield 2021-11-20 18:08:18 PST 
Oh, I think I see what's happening.

Just like:
font: bold 48px 'Helvetica'; font-weight: normal;
causes the text to be rendered with normal weight, not bold,

font: -apple-system-body; font-weight: normal;
causes text to be rendered with normal weight, not whatever weight -apple-system-body corresponds to.
Comment 9 Myles C. Maxfield 2021-11-23 14:09:50 PST 
Aha! CFDictionaryGetValue(CTFontCopyTraits(CTFontCreateWithFontDescriptor(CTFontDescriptorCreateWithTextStyle(kCTUIFontTextStyleBody, contentSizeCategory(), nullptr))), kCTFontWeightTrait) is returning 0
Comment 10 Myles C. Maxfield 2021-11-23 14:23:08 PST 
I can just use kCTFontCSSWeightAttribute instead.
Comment 11 Myles C. Maxfield 2021-11-23 14:32:30 PST 
Created attachment 445050 [details]
Patch
Comment 12 Myles C. Maxfield 2021-11-23 17:13:14 PST 
Created attachment 445055 [details]
Patch
Comment 13 Cameron McCormack (:heycam) 2021-11-23 17:55:38 PST 
Comment on attachment 445055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445055&action=review

> Source/WebCore/rendering/RenderThemeCocoa.mm:287
> +    CFNumberRef resultRef = static_cast<CFNumberRef>(CTFontCopyAttribute(font, kCTFontCSSWeightAttribute));

"auto resultRef"
Comment 14 Myles C. Maxfield 2021-11-29 11:51:24 PST 
Comment on attachment 445055 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445055&action=review

>> Source/WebCore/rendering/RenderThemeCocoa.mm:287
>> +    CFNumberRef resultRef = static_cast<CFNumberRef>(CTFontCopyAttribute(font, kCTFontCSSWeightAttribute));
> 
> "auto resultRef"

I wrote a memory leak :( this line needs an adoptCF().
Comment 15 Myles C. Maxfield 2021-11-29 12:25:26 PST 
Committed r286254 (244617@main): <https://commits.webkit.org/244617@main>