Bug 204276 - Implement the 'ic' unit from CSS Values 4
Summary: Implement the 'ic' unit from CSS Values 4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: GoodFirstBug, InRadar, WebExposed, WPTImpact
Depends on:
Blocks:
 
Reported: 2019-11-16 12:20 PST by Simon Fraser (smfr)
Modified: 2021-09-29 18:36 PDT (History)
18 users (show)

See Also:


Attachments
Patch (30.77 KB, patch)
2021-09-08 22:46 PDT, Kiet Ho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.78 KB, patch)
2021-09-08 22:54 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (29.23 KB, patch)
2021-09-09 00:27 PDT, Kiet Ho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.81 KB, patch)
2021-09-09 03:10 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (41.68 KB, patch)
2021-09-11 01:16 PDT, Kiet Ho
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (41.84 KB, patch)
2021-09-11 05:18 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (52.58 KB, patch)
2021-09-23 01:43 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (55.36 KB, patch)
2021-09-23 03:48 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (55.85 KB, patch)
2021-09-23 23:25 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff
Patch (55.50 KB, patch)
2021-09-29 01:07 PDT, Kiet Ho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-11-16 12:20:39 PST
https://drafts.csswg.org/css-values-4/#ic
Comment 1 Radar WebKit Bug Importer 2019-11-16 12:21:11 PST
<rdar://problem/57256127>
Comment 2 Kiet Ho 2021-09-08 22:46:31 PDT
Created attachment 437711 [details]
Patch

Patch. Reviews appreciated, after which I'll remove all logging
Comment 3 EWS Watchlist 2021-09-08 22:47:46 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 Kiet Ho 2021-09-08 22:54:13 PDT
Created attachment 437712 [details]
Patch

Patch. Reviews appreciated, after which I'll remove all logging
Comment 5 Kiet Ho 2021-09-09 00:27:26 PDT
Created attachment 437717 [details]
Patch

Patch. Removed all logging to avoid compilatione errors.
Comment 6 Kiet Ho 2021-09-09 03:10:07 PDT
Created attachment 437725 [details]
Patch

Fix various runtime crashes.
Comment 7 Darin Adler 2021-09-09 11:02:46 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
> +    case CSSUnitType::CSS_ICS:                          return 110;

Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.

This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.

> Source/WebCore/platform/graphics/Font.cpp:170
> +    auto* glyphPageCjkWater = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater));
> +    if (glyphPageCjkWater) {

Please scope the variable into the if. I suggest naming it just "page".

    if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {

But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.

I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.

> Source/WebCore/platform/graphics/Font.cpp:171
> +        auto cjkWaterGlyph = glyphPageCjkWater->glyphDataForCharacter(cjkWater).glyph;

I suggest naming the local variable just glyphData.

> Source/WebCore/platform/graphics/FontMetrics.h:152
> +        m_ideogramWidth.reset();

We typically write:

    m_ideogramWidth = std::nullopt;

Not sure if it’s better style.

> Source/WebCore/platform/graphics/FontMetrics.h:173
> +    std::optional<float> m_ideogramWidth { };

No need for that "{ }".
Comment 8 Darin Adler 2021-09-09 11:03:07 PDT
Good work, I think this is close to right!
Comment 9 Darin Adler 2021-09-09 13:39:11 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:620
> +        if (std::optional<float> width = fontMetrics->ideogramWidth())
> +            return (*width) * value;

I would write:

    if (auto width = fontMetrics->ideogramWidth())
        return *width * value;
Comment 10 Kiet Ho 2021-09-10 04:11:06 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>> Source/WebCore/css/CSSPrimitiveValue.cpp:620
>> +            return (*width) * value;
> 
> I would write:
> 
>     if (auto width = fontMetrics->ideogramWidth())
>         return *width * value;

This is a C++ habit of mine: wrapping everything in parentheses so I won't have to remember operator precedence. But will change.

>> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
>> +    case CSSUnitType::CSS_ICS:                          return 110;
> 
> Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.
> 
> This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.

To be honest, before your comment I didn't really know which value is right here either. I could've returned CSS_UNKNOWN, but no units did that so I decided to follow the existing "convention". Will change regardless, but in that case we wouldn't need to handle it in DeprecaredCSSOMPrimitiveValue::getFloatValue() right?

>> Source/WebCore/platform/graphics/Font.cpp:170
>> +    if (glyphPageCjkWater) {
> 
> Please scope the variable into the if. I suggest naming it just "page".
> 
>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
> 
> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
> 
> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.

I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.

>> Source/WebCore/platform/graphics/Font.cpp:171
>> +        auto cjkWaterGlyph = glyphPageCjkWater->glyphDataForCharacter(cjkWater).glyph;
> 
> I suggest naming the local variable just glyphData.

Will change.

>> Source/WebCore/platform/graphics/FontMetrics.h:152
>> +        m_ideogramWidth.reset();
> 
> We typically write:
> 
>     m_ideogramWidth = std::nullopt;
> 
> Not sure if it’s better style.

Will change.

>> Source/WebCore/platform/graphics/FontMetrics.h:173
>> +    std::optional<float> m_ideogramWidth { };
> 
> No need for that "{ }".

Will change.
Comment 11 Darin Adler 2021-09-10 10:04:55 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>> Source/WebCore/css/DeprecatedCSSOMPrimitiveValue.cpp:46
>>> +    case CSSUnitType::CSS_ICS:                          return 110;
>> 
>> Why is 110 right? I am pretty sure instead we want CSS_UNKNOWN. Which test covers this? We should not write code that is not covered by any test.
>> 
>> This deprecated API should not be expanded to cover each new unit type; eventually most cases from this switch statement need to be removed. Please observe the FIXME statement above.
> 
> To be honest, before your comment I didn't really know which value is right here either. I could've returned CSS_UNKNOWN, but no units did that so I decided to follow the existing "convention". Will change regardless, but in that case we wouldn't need to handle it in DeprecaredCSSOMPrimitiveValue::getFloatValue() right?

Yes.

But we need to add tests rather than make the changes based on theory.
Comment 12 Darin Adler 2021-09-10 10:05:37 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>> Source/WebCore/platform/graphics/Font.cpp:170
>>> +    if (glyphPageCjkWater) {
>> 
>> Please scope the variable into the if. I suggest naming it just "page".
>> 
>>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
>> 
>> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
>> 
>> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.
> 
> I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.

That sounds like a good idea, although you’d probably have to use a std::optional for the return value?
Comment 13 Kiet Ho 2021-09-10 21:17:16 PDT
Comment on attachment 437725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437725&action=review

>>>> Source/WebCore/platform/graphics/Font.cpp:170
>>>> +    if (glyphPageCjkWater) {
>>> 
>>> Please scope the variable into the if. I suggest naming it just "page".
>>> 
>>>     if (auto page = glyphPage(GlyphPage::pageNumberForCodePoint(cjkWater))) {
>>> 
>>> But also, this code needs a "why" comment. It’s not at all clear that calling setIdeogramWidth with the width of the cjkWater glyph is a good strategy for getting the widths of all ideograms, so we need a brief comment explaining the strategy.
>>> 
>>> I’m also a bit surprised that getting the page and then the data has to be done like that rather than a function that combines both operations.
>> 
>> I'll scope the variable and some comments. If you want to, I can have a static function to do the whole "getting the page then font then glyph width" dance, because it's also used to get the width for zero and space glyph.
> 
> That sounds like a good idea, although you’d probably have to use a std::optional for the return value?

Right, I'll keep that in mind.
Comment 14 Kiet Ho 2021-09-11 01:16:52 PDT
Created attachment 437944 [details]
Patch

Notes:
1. I think I'll need more guidance on what to do regarding DeprecatedCSSOMPrimitive Value
2. Font::platformGlyphInit() requires both the glyph number and width of the space character, therefore even if there's a method to get the glyph number then width, we'd have to duplicate the logic to get the glyph number regardless. I decided to just clean up the method and surrounding areas.
Comment 15 Kiet Ho 2021-09-11 05:18:29 PDT
Created attachment 437950 [details]
Patch

Notes:\n1. I think I'll need more guidance on what to do regarding DeprecatedCSSOMPrimitive Value\n2. Font::platformGlyphInit() requires both the glyph number and width of the space character, therefore even if there's a method to get the glyph number then width, we'd have to duplicate the logic to get the glyph number regardless. I decided to just clean up the method and surrounding areas.
Comment 16 Darin Adler 2021-09-12 16:57:29 PDT
(In reply to Kiet Ho from comment #14)
> 1. I think I'll need more guidance on what to do regarding
> DeprecatedCSSOMPrimitive Value

In primitiveType, add a case to the switch statement and return CSS_UNKNOWN. In getFloatValue, getStringValue, getCounterValue, getRectValue, and getRGBColorValue, do not change the code at all.

More importantly, write a test verifying the behavior of these functions. Look at the fast/css/getFloatValueForUnit.html and the tests in fast/css/getComputedStyle and fast/css/resources/CSSPrimitiveValue-exceptions.js for examples of some tests that cover what these functions do.
Comment 17 Kiet Ho 2021-09-23 01:43:56 PDT
Created attachment 439023 [details]
Patch

WIP, making sure that all layout tests pass
Comment 18 Kiet Ho 2021-09-23 03:48:44 PDT
Created attachment 439032 [details]
Patch
Comment 19 Kiet Ho 2021-09-23 03:54:12 PDT
I saw two things while making CSSPrimitiveValue tests:

* In CSSUnits.cpp, WebCore::unitCategory() does not return CSSUnitCategory::Length for a bunch of units (ch, rem, ...)

* CSSPrimitiveValue.getFloatValue() in JS uses CSSPrimitiveValue::doubleValueInternal() in CSSPrimitiveValue.cpp. This relies on CSSPrimitiveValue::conversionToCanonicalUnitsScaleFactor(), which does not have enough information (correct me if I'm wrong) to convert from/to ic units. Thus getFloatValue() is broken for ic.

Opinions? Should I fixed both of them?
Comment 20 Simon Fraser (smfr) 2021-09-23 11:50:13 PDT
Comment on attachment 439032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439032&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:1051
>      case CSSUnitType::CSS_CHS:
>          return formatNumberValue("ch");
> +    case CSSUnitType::CSS_ICS:
> +        return formatNumberValue("ic");

The "CHS"  vs "ch" and "ICS" vs "ic" is confusing. Why aren't the unit types CSS_CH and CSS_IC?

> LayoutTests/imported/w3c/ChangeLog:8
> +

Please describe why you had to change the WPT.
Comment 21 Kiet Ho 2021-09-23 11:55:36 PDT
Comment on attachment 439032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439032&action=review

>> Source/WebCore/css/CSSPrimitiveValue.cpp:1051
>> +        return formatNumberValue("ic");
> 
> The "CHS"  vs "ch" and "ICS" vs "ic" is confusing. Why aren't the unit types CSS_CH and CSS_IC?

I have no explanation other than I just followedcthe existing convention for CSS_CHS. I'll rename CSS_ICS to CSS_IC. As for CSS_CHS, maybe that's for another patch.

>> LayoutTests/imported/w3c/ChangeLog:8
>> +
> 
> Please describe why you had to change the WPT.

Yeah, I'll fill in the changelogs after everything else checks out.
Comment 22 Kiet Ho 2021-09-23 23:25:21 PDT
Created attachment 439128 [details]
Patch

Should be final
Comment 23 Myles C. Maxfield 2021-09-24 13:16:11 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/platform/graphics/FontMetrics.h:173
> +    std::optional<float> m_ideogramWidth;

Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
Comment 24 Myles C. Maxfield 2021-09-24 13:16:55 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/ChangeLog:8
> +

Ideally there would be a description of the change here.
Comment 25 Kiet Ho 2021-09-24 13:31:38 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>> Source/WebCore/ChangeLog:8
>> +
> 
> Ideally there would be a description of the change here.

I'll put one in, I thought the description above and changes for each method are enough.

>> Source/WebCore/platform/graphics/FontMetrics.h:173
>> +    std::optional<float> m_ideogramWidth;
> 
> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?

I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?
Comment 26 Myles C. Maxfield 2021-09-24 13:55:16 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>> +    std::optional<float> m_ideogramWidth;
>> 
>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
> 
> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?

That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.

If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.
Comment 27 Kiet Ho 2021-09-24 16:24:30 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>>> +    std::optional<float> m_ideogramWidth;
>>> 
>>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
>> 
>> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?
> 
> That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.
> 
> If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.

I agree, I'll fix it as you said.
Comment 28 Kiet Ho 2021-09-24 16:33:54 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

>>>>> Source/WebCore/platform/graphics/FontMetrics.h:173
>>>>> +    std::optional<float> m_ideogramWidth;
>>>> 
>>>> Why is this an optional? Why not have platformGlyphInit() implement the fallback behavior, rather than computeUnzoomedNonCalcLengthDouble() implement the fallback behavior?
>>> 
>>> I just feel like Font should truthfully indicate whether the ideogram width is available or not, so its users can make the correct decision. Right now computeUnzoomedNonCalcLengthDouble() falls back to 1em, but maybe in the future some other code might want to use m_ideogramWidth for something else?
>> 
>> That doesn't make any sense to me. The spec dictates a particular fallback behavior. There is one client, which implements the fallback behavior. Storing an optional stores an unnecessary extra 4 bytes per font.
>> 
>> If there were multiple clients who wanted different behavior here, that would make sense. But there isn't, so better to save those 4 bytes.
> 
> I agree, I'll fix it as you said.

I don't think it's possible to implement the fallback behavior inside Font/FontMetrics, because the fallback metric (1em) comes from FontCascadeDescription.m_specifiedSize, which depends on whatever font size being specified in CSS. This is not available when the Font is initialized.
Comment 29 Myles C. Maxfield 2021-09-24 18:00:28 PDT
Comment on attachment 439128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439128&action=review

> Source/WebCore/platform/graphics/Font.cpp:166
> +    // to support the ic unit.

I think here you would just say m_fontMetrics.setIdeogramWidth(platformData().size()); and that would end up setting it to 1em.
Comment 30 Kiet Ho 2021-09-29 01:07:50 PDT
Created attachment 439579 [details]
Patch
Comment 31 Myles C. Maxfield 2021-09-29 07:13:30 PDT
Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

> Source/WebCore/platform/graphics/Font.h:-138
> -    FontMetrics& fontMetrics() { return m_fontMetrics; }

Are these unused?

> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-001-expected.html:-6
> -svg { width: 10ic; }

Are these being updated upstream too?
Comment 32 Kiet Ho 2021-09-29 11:04:55 PDT
Comment on attachment 439579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439579&action=review

>> Source/WebCore/platform/graphics/Font.h:-138
>> -    FontMetrics& fontMetrics() { return m_fontMetrics; }
> 
> Are these unused?

It appears to be the case (because we also have a const getter), but if you want to be on the safe side I'll keep it.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-values/ic-unit-001-expected.html:-6
>> -svg { width: 10ic; }
> 
> Are these being updated upstream too?

Yes, I'll upstream it when this patch passes CQ.
Comment 33 EWS 2021-09-29 16:31:41 PDT
Committed r283279 (242306@main): <https://commits.webkit.org/242306@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439579 [details].