Bug 226272 - Support calc() on components inside relative color syntax colors
Summary: Support calc() on components inside relative color syntax colors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on: 226477
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-26 08:26 PDT by Sam Weinig
Modified: 2021-07-03 10:06 PDT (History)
10 users (show)

See Also:


Attachments
Patch (37.43 KB, patch)
2021-05-29 21:28 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (87.47 KB, patch)
2021-05-30 11:54 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (86.67 KB, patch)
2021-05-30 11:56 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (86.71 KB, patch)
2021-05-30 12:03 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (86.71 KB, patch)
2021-05-30 12:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (91.29 KB, patch)
2021-05-30 15:20 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (90.42 KB, patch)
2021-05-31 18:47 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-05-26 08:26:01 PDT
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.
Comment 1 Sam Weinig 2021-05-29 21:28:47 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2021-05-30 11:54:43 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2021-05-30 11:56:25 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2021-05-30 12:03:47 PDT
Created attachment 430142 [details]
Patch
Comment 5 Sam Weinig 2021-05-30 12:18:42 PDT
Created attachment 430143 [details]
Patch
Comment 6 Darin Adler 2021-05-30 14:01:16 PDT
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.
Comment 7 Sam Weinig 2021-05-30 15:01:40 PDT
(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.
Comment 8 Sam Weinig 2021-05-30 15:14:18 PDT
Meta question:

Is it time to split up CSSPropertyParserHelpers.h/cpp into some meaningful structure?
Comment 9 Sam Weinig 2021-05-30 15:20:56 PDT
Created attachment 430152 [details]
Patch
Comment 10 EWS 2021-05-30 17:29:25 PDT
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].
Comment 11 Radar WebKit Bug Importer 2021-05-30 17:30:22 PDT
<rdar://problem/78669976>
Comment 12 WebKit Commit Bot 2021-05-31 18:14:31 PDT
Re-opened since this is blocked by bug 226477
Comment 13 Sam Weinig 2021-05-31 18:41:18 PDT
Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why?
Comment 14 Sam Weinig 2021-05-31 18:43:46 PDT
(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.
Comment 15 Sam Weinig 2021-05-31 18:47:19 PDT
Created attachment 430222 [details]
Patch
Comment 16 EWS 2021-06-01 06:50:23 PDT
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].