Bug 226272 – Support calc() on components inside relative color syntax colors

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sam Weinig]

calc() on components inside relative color syntax colors currently does not work:

https://codepen.io/leaverou/pen/VwpWNgL?editors=0100&fbclid=IwAR18C0aanphaAXkutH2oXzZp2bgsNHhWjwl0NqG8wz7D1rfKZHlq2iPg5xw

(Test case by Lea Verou).

To make this work, we will need to add support to our calc() implementation to allow injection of variables, which hopefully will be relatively straight forward, though we will see.

[Sam Weinig]

Created attachment 430128 [details]
Patch

[Sam Weinig]

Created attachment 430139 [details]
Patch

[Sam Weinig]

Created attachment 430141 [details]
Patch

[Sam Weinig]

Created attachment 430142 [details]
Patch

[Sam Weinig]

Created attachment 430143 [details]
Patch

[Darin Adler]

Comment on attachment 430143 [details]
Patch

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

Some of the code feels a little bit copy/pastey. Two similar functions.

> Source/WebCore/css/makevalues.pl:212
> +template<> struct DefaultHash<WebCore::CSSValueID> : IntHash<unsigned> { };
> +template<> struct HashTraits<WebCore::CSSValueID> : GenericHashTraits<WebCore::CSSValueID> {

Explicitly doing this here is a little annoying. We don’t have to do that when we use an integer type.

Can we teach HashFunctions.h and HashTraits.h to default to hashing enums based on their underlying integer types? I think that using uint16_t::max would probably be just as good as lastCSSValueKeyword + 1.

> Source/WebCore/css/calc/CSSCalcValue.cpp:316
> +    return CSSCalcValue::create(function, tokens, destinationCategory, range, { });

Don’t need to write CSSCalcValue here.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:586
> +        CalcParser angleCalcParser(range, CalculationCategory::Angle, valueRange);

Should we scope this so the first parser is destroyed before the next one is allocated? Maybe not needed

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:588
> +        if (const CSSCalcValue* calculation = angleCalcParser.value()) {
> +            if (calculation->category() == CalculationCategory::Angle)

auto, also could do a single if:

    if (auto calculation = angleCalcParser.value(); calculation && calculation->category() == CalculationCategory::Angle)

Seems like a good style.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:594
> +        if (const CSSCalcValue* calculation = percentCalcParser.value()) {
> +            if (calculation->category() == CalculationCategory::Percent)

Ditto.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:791
> +        if (std::isinf(token.numericValue()))

Use !isfinite instead of isinf? What handling do we want for NaN?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:934
> +        if (std::isinf(token.numericValue()))

Same !isfinite thought.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1237
> +    auto normalizedSaturation = clampTo(*saturation, 0.0, 100.0);
> +    auto normalizedLightness = clampTo(*lightness, 0.0, 100.0);
> +    auto normalizedAlpha = clampTo(*alpha, 0.0, 1.0);

For all these calls to clampTo, I suggest using std::clamp instead. Our own special WebKit clampTo is only needed when we are mixing types or when we need defined behavior if max < min.

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 430143 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430143&action=review
> 
> Some of the code feels a little bit copy/pastey. Two similar functions.
> 
> > Source/WebCore/css/makevalues.pl:212
> > +template<> struct DefaultHash<WebCore::CSSValueID> : IntHash<unsigned> { };
> > +template<> struct HashTraits<WebCore::CSSValueID> : GenericHashTraits<WebCore::CSSValueID> {
> 
> Explicitly doing this here is a little annoying. We don’t have to do that
> when we use an integer type.
> 
> Can we teach HashFunctions.h and HashTraits.h to default to hashing enums
> based on their underlying integer types? I think that using uint16_t::max
> would probably be just as good as lastCSSValueKeyword + 1.

I was a little surprised it was needed. We have a struct called StrongEnumHashTraits that we can probably use that does std::numeric_limits<UnderlyingType>::max() for empty() and std::numeric_limits<UnderlyingType>::max() - 1, for deleted. I will try to just use that.

> 
> > Source/WebCore/css/calc/CSSCalcValue.cpp:316
> > +    return CSSCalcValue::create(function, tokens, destinationCategory, range, { });
> 
> Don’t need to write CSSCalcValue here.

Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:586
> > +        CalcParser angleCalcParser(range, CalculationCategory::Angle, valueRange);
> 
> Should we scope this so the first parser is destroyed before the next one is
> allocated? Maybe not needed.

Not sure it gets us anything. That said, there are some optimizations we should consider here in the future. We could hoist the CSSCalcValue::isCalcFunction() check out of the CalcParser constructor, and given we don't ever want a CSSCalcValue, we could probably bypass its allocation entirely by having a static way to parse the calc() into the desired raw value.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:588
> > +        if (const CSSCalcValue* calculation = angleCalcParser.value()) {
> > +            if (calculation->category() == CalculationCategory::Angle)
> 
> auto, also could do a single if:
> 
>     if (auto calculation = angleCalcParser.value(); calculation &&
> calculation->category() == CalculationCategory::Angle)
> 
> Seems like a good style.

I added a consumeValueIfCategory(CalculationCategory) that simplifies things for a bunch of call sites. 

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:594
> > +        if (const CSSCalcValue* calculation = percentCalcParser.value()) {
> > +            if (calculation->category() == CalculationCategory::Percent)
> 
> Ditto.
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:791
> > +        if (std::isinf(token.numericValue()))
> 
> Use !isfinite instead of isinf? What handling do we want for NaN?

Not sure, but this is copied code, so I don't want to change it in this patch. Will work on unifying these checks and making sure we have tests.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:934
> > +        if (std::isinf(token.numericValue()))
> 
> Same !isfinite thought.
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1237
> > +    auto normalizedSaturation = clampTo(*saturation, 0.0, 100.0);
> > +    auto normalizedLightness = clampTo(*lightness, 0.0, 100.0);
> > +    auto normalizedAlpha = clampTo(*alpha, 0.0, 1.0);
> 
> For all these calls to clampTo, I suggest using std::clamp instead. Our own
> special WebKit clampTo is only needed when we are mixing types or when we
> need defined behavior if max < min.

Ok. Fixed.

Thanks for review.

[Sam Weinig]

Meta question:

Is it time to split up CSSPropertyParserHelpers.h/cpp into some meaningful structure?

[Sam Weinig]

Created attachment 430152 [details]
Patch

[EWS]

Committed r278261 (238298@main): <https://commits.webkit.org/238298@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78669976>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 226477

[Sam Weinig]

Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why?

[Sam Weinig]

(In reply to Sam Weinig from comment #13)
> Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why?

And why only "win" and not "wincairo"?

Well, that was just an optimization to avoid a header include, so I can just leave it for now.

[Sam Weinig]

Created attachment 430222 [details]
Patch

[EWS]

Committed r278304 (238341@main): <https://commits.webkit.org/238341@main>

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