RESOLVED FIXED 236631
Change the canonical unit for time category from ms to s. 
https://bugs.webkit.org/show_bug.cgi?id=236631
Summary Change the canonical unit for time category from ms to s.
Joonghun Park
Reported 2022-02-14 20:54:03 PST
According to the spec, https://drafts.csswg.org/css-values-4/#time, the canonical unit for <time> is Seconds.
Attachments
Patch (2.53 KB, patch)
2022-02-14 20:59 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (9.40 KB, patch)
2022-02-15 00:26 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (16.71 KB, patch)
2022-02-16 06:06 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (16.71 KB, patch)
2022-02-16 06:07 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (17.89 KB, patch)
2022-02-16 07:01 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Patch (19.64 KB, patch)
2022-02-16 15:16 PST, Joonghun Park 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Joonghun Park
Comment 1 2022-02-14 20:59:31 PST
Created attachment 451990 [details] Patch
Joonghun Park
Comment 2 2022-02-15 00:26:29 PST
Created attachment 451999 [details] Patch
Darin Adler
Comment 3 2022-02-15 06:03:17 PST
Comment on attachment 451999 [details] Patch I am really surprised that this behavior is not tested by any WPT test. Assuming this can affect browser interoperability, we need to get something into WPT. Or maybe this only affects the legacy CSS object model and that is not well tested in WPT? If this does only affect the legacy CSS object model then I wonder what the odds are that this breaks some existing content accidentally relying on this. How does this behave in other web browsers?
Joonghun Park
Comment 4 2022-02-16 05:52:57 PST
In case of Blink, I landed a change, https://chromium-review.googlesource.com/c/chromium/src/+/3460189, which has the same change to this patch. So it lowers interoperablity risk to some extent. And in wpt aspects, css/css-values/minmax-time-computed.html css/css-values/minmax-time-serialize.html should check the behavior(The change in Blink changed the result of the 2 wpt tests from fail to pass as expected.) After I read your comment, I found that I missed out to change case CalculationCategory::Time: return CSSUnitType::CSS_MS; canonicalUnitTypeForCalculationCategory() in CSSCalcCategoryMapping.cpp. I will apply this change too. Thank you for your review!
Joonghun Park
Comment 5 2022-02-16 06:06:08 PST
Created attachment 452189 [details] Patch
Joonghun Park
Comment 6 2022-02-16 06:07:25 PST
Created attachment 452190 [details] Patch
Joonghun Park
Comment 7 2022-02-16 07:01:07 PST
Created attachment 452194 [details] Patch
Joonghun Park
Comment 8 2022-02-16 15:16:09 PST
Created attachment 452254 [details] Patch
EWS
Comment 9 2022-02-16 22:06:25 PST
Committed r289997 (247383@main): <https://commits.webkit.org/247383@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452254 [details].
Radar WebKit Bug Importer
Comment 10 2022-02-16 22:07:20 PST
<rdar://problem/89068282>
Note You need to log in before you can comment on or make changes to this bug.