Bug 214230

Summary: font-weight should always serialize as a number
Product: WebKit Reporter: Takeshi Kurosawa <taken.spc>
Component: CSSAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, bfulgham, calvaris, cfleizach, dbarton, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fred.wang, glenn, gsnedders, gyuyoung.kim, jcraig, jdiggs, jer.noble, koivisto, macpherson, menard, mifenton, mmaxfield, philipj, samuel_white, sergio, sime.vidas, simon.fraser, twilco.o, tyler_w, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Takeshi Kurosawa 2020-07-11 23:16:14 PDT 
Created attachment 404087 [details]
Test case

The font-weight should always serialize as a number (400, 700 and so on). 

WebKit uses keyword ("normal" or "bold") when possible and number otherwise (300, 600 and son on). This behavior has some drawbacks:


1. It is a bit harder to get differences of computed values in animations


```
const {fontWeight: oldFontWeight} = window.getComputedStyle(element);
// do some stuff
const {fontWeight} = window.getComputedStyle(element));
const diff = parseInt(oldFontWeight, 10) - parseInt(fontWeight, 10);
```


Expected behavior
If oldFontWeight is "300" and fontWeight is "600" then the diff is 300.
If oldFontWeight is "400" and fontWeight is "700" then the diff is 300.

Actual behavior
If oldFontWeight is "300" and fontWeight is "600" then the diff  is 300.😀 
If oldFontWeight is "normal" and fontWeight is "bold" then the diff is NaN.🥺


2. Inconsistency with specs

CSS Fonts Module Level 3 defines computed value of font-weight is "numeric"
https://www.w3.org/TR/2018/REC-css-fonts-3-20180920/#propdef-font-weight

CSS Fonts Module Level 4 defines computed value of font-weight is "number"
https://drafts.csswg.org/css-fonts/#propdef-font-weight


3. Inconsistency with other browsers

- IE 11: always use a number
- Chrome: always uses a number
- Firefox always uses a number

WebKit should always use number for font-weight.
Comment 1 Sam Sneddon [:gsnedders] 2020-07-15 07:54:08 PDT 
This accounts for all of the failures in https://wpt.fyi/results/css/css-fonts/parsing/font-weight-computed.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned

I presume there's some compat risk here (esp. with regards to WKWebViews and Books), even with other UAs already doing this per spec.
Comment 2 Radar WebKit Bug Importer 2020-07-15 13:23:23 PDT 
<rdar://problem/65623540>
Comment 3 Tyler Wilcock 2020-09-13 11:57:03 PDT 
Created attachment 408661 [details]
Patch
Comment 4 Myles C. Maxfield 2020-09-13 15:55:47 PDT 
The patch looks pretty good, but it looks like there need to be some test updates before it can be committed.
Comment 5 Tyler Wilcock 2020-09-17 23:36:33 PDT 
Created attachment 409107 [details]
Patch

This patch should be closer, but I'm still expecting some failures.  I want to see what the results in EWS look like.
Comment 6 Tyler Wilcock 2020-09-18 10:37:32 PDT 
This patch has resulted in pixel test changes that I'm not sure are right.

For example, look at the layout test results for ios-wk2 [1].  In the text snapshot of `editing/pasteboard/3976872.html` [2], <span>s are now being generated instead of <b>s, which in itself is not necessarily incorrect, assuming the span is styled correctly.  But in the image diff [3], the result box now ends up being highlighted.  This actually matches Chromium's behavior for this test.

One that certainly seems incorrect, though, is `editing/pasteboard/paste-match-style-001.html`, where the image diff [4] shows a space has been added between the 'a' and the 'b' characters.  The text diff [5] shows both a <b> *and* a <span> are being generated.  Interestingly, neither Chromium nor Gecko render the 'b' character at all for this test.

It seems like these inconsistencies are all related to usage of https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand, which is apparently deprecated.  Should I just update the expected results with these new ones, or do we need to dig deeper on any of these?


[1]: https://ews-build.s3-us-west-2.amazonaws.com/iOS-13-Simulator-WK2-Tests-EWS/r409107-26225/results.html

[2]: https://ews-build.s3-us-west-2.amazonaws.com/iOS-13-Simulator-WK2-Tests-EWS/r409107-26225/editing/pasteboard/3976872-pretty-diff.html

[3]: https://ews-build.s3-us-west-2.amazonaws.com/iOS-13-Simulator-WK2-Tests-EWS/r409107-26225/retries/editing/pasteboard/3976872-diffs.html

[4]: https://ews-build.s3-us-west-2.amazonaws.com/iOS-13-Simulator-WK2-Tests-EWS/r409107-26225/retries/editing/pasteboard/paste-match-style-001-diffs.html

[5]: https://ews-build.s3-us-west-2.amazonaws.com/iOS-13-Simulator-WK2-Tests-EWS/r409107-26225/editing/pasteboard/paste-match-style-001-pretty-diff.html
Comment 7 Sam Sneddon [:gsnedders] 2021-07-28 04:13:32 PDT 
*** Bug 228540 has been marked as a duplicate of this bug. ***
Comment 8 Myles C. Maxfield 2021-09-14 12:53:00 PDT 
This hasn't been touched in about a year, and the Assignee field is "Nobody," so if nobody minds, I'll pick this up and continue working on it.
Comment 9 Tyler Wilcock 2021-09-14 13:12:23 PDT 
Go for it!
Comment 10 Myles C. Maxfield 2021-09-14 17:49:16 PDT 
Created attachment 438193 [details]
Patch
Comment 11 Myles C. Maxfield 2021-09-14 23:32:26 PDT 
Created attachment 438216 [details]
Patch
Comment 12 Myles C. Maxfield 2021-09-15 00:13:02 PDT 
Created attachment 438218 [details]
Patch
Comment 13 Myles C. Maxfield 2021-09-15 00:22:36 PDT 
editing/mac/spelling/autocorrection-blockquote-crash.html fails for me even on a clean build, without this patch applied 🤔
Comment 14 Myles C. Maxfield 2021-09-15 00:24:54 PDT 
Oh, it's marked as a failure in LayoutTests/platform/mac-wk2/TestExpectations
Comment 15 Myles C. Maxfield 2021-09-15 02:57:35 PDT 
HTMLElementEquivalent::valueIsPresentInStyle() / htmlElementEquivalents() needs to be updated, too.
Comment 16 Myles C. Maxfield 2021-09-15 03:17:14 PDT 
Created attachment 438235 [details]
Patch
Comment 17 Myles C. Maxfield 2021-09-15 11:24:05 PDT 
Created attachment 438264 [details]
Patch
Comment 18 EWS 2021-09-16 03:55:55 PDT 
Committed r282545 (241743@main): <https://commits.webkit.org/241743@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438264 [details].
Comment 19 Brent Fulgham 2022-02-04 13:55:30 PST 
This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.