Bug 233070 - [Cocoa] REGRESSION(r281291): Text Style fonts don't have the correct weight set
Summary: [Cocoa] REGRESSION(r281291): Text Style fonts don't have the correct weight set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-12 13:30 PST by Myles C. Maxfield
Modified: 2021-11-29 12:25 PST (History)
8 users (show)

See Also:

Attachments
Needs test (10.58 KB, patch)
2021-11-12 13:37 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Needs test (10.58 KB, patch)
2021-11-12 13:39 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (12.58 KB, patch)
2021-11-12 14:35 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (17.42 KB, patch)
2021-11-18 23:48 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2021-11-19 15:09 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (12.96 KB, patch)
2021-11-23 14:32 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2021-11-23 17:13 PST, Myles C. Maxfield 		heycam: review+
Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>