Bug 214230 - font-weight should always serialize as a number
Summary: font-weight should always serialize as a number
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 228540 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-11 23:16 PDT by Takeshi Kurosawa
Modified: 2021-09-16 03:56 PDT (History)
31 users (show)

See Also:


Attachments
Test case (759 bytes, text/html)
2020-07-11 23:16 PDT, Takeshi Kurosawa
no flags Details
Patch (1.68 KB, patch)
2020-09-13 11:57 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (355.85 KB, patch)
2020-09-17 23:36 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (301.43 KB, patch)
2021-09-14 17:49 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (330.69 KB, patch)
2021-09-14 23:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (331.77 KB, patch)
2021-09-15 00:13 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (335.56 KB, patch)
2021-09-15 03:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (339.72 KB, patch)
2021-09-15 11:24 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

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