Bug 236631 - Change the canonical unit for time category from ms to s.
Summary: Change the canonical unit for time category from ms to s.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-14 20:54 PST by Joonghun Park
Modified: 2022-02-16 22:07 PST (History)
9 users (show)

See Also:

Attachments
Patch (2.53 KB, patch)
2022-02-14 20:59 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2022-02-15 00:26 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Patch (16.71 KB, patch)
2022-02-16 06:06 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Patch (16.71 KB, patch)
2022-02-16 06:07 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Patch (17.89 KB, patch)
2022-02-16 07:01 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Patch (19.64 KB, patch)
2022-02-16 15:16 PST, Joonghun Park 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) 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 Joonghun Park 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.
Comment 1 Joonghun Park 2022-02-14 20:59:31 PST 
Created attachment 451990 [details]
Patch
Comment 2 Joonghun Park 2022-02-15 00:26:29 PST 
Created attachment 451999 [details]
Patch
Comment 3 Darin Adler 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?
Comment 4 Joonghun Park 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!
Comment 5 Joonghun Park 2022-02-16 06:06:08 PST 
Created attachment 452189 [details]
Patch
Comment 6 Joonghun Park 2022-02-16 06:07:25 PST 
Created attachment 452190 [details]
Patch
Comment 7 Joonghun Park 2022-02-16 07:01:07 PST 
Created attachment 452194 [details]
Patch
Comment 8 Joonghun Park 2022-02-16 15:16:09 PST 
Created attachment 452254 [details]
Patch
Comment 9 EWS 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].
Comment 10 Radar WebKit Bug Importer 2022-02-16 22:07:20 PST 
<rdar://problem/89068282>