WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
226272
Support calc() on components inside relative color syntax colors
https://bugs.webkit.org/show_bug.cgi?id=226272
Summary
Support calc() on components inside relative color syntax colors
Sam Weinig
Reported
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.
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2021-05-29 21:28:47 PDT
Comment hidden (obsolete)
Created
attachment 430128
[details]
Patch
Sam Weinig
Comment 2
2021-05-30 11:54:43 PDT
Comment hidden (obsolete)
Created
attachment 430139
[details]
Patch
Sam Weinig
Comment 3
2021-05-30 11:56:25 PDT
Comment hidden (obsolete)
Created
attachment 430141
[details]
Patch
Sam Weinig
Comment 4
2021-05-30 12:03:47 PDT
Created
attachment 430142
[details]
Patch
Sam Weinig
Comment 5
2021-05-30 12:18:42 PDT
Created
attachment 430143
[details]
Patch
Darin Adler
Comment 6
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.
Sam Weinig
Comment 7
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.
Sam Weinig
Comment 8
2021-05-30 15:14:18 PDT
Meta question: Is it time to split up CSSPropertyParserHelpers.h/cpp into some meaningful structure?
Sam Weinig
Comment 9
2021-05-30 15:20:56 PDT
Created
attachment 430152
[details]
Patch
EWS
Comment 10
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]
.
Radar WebKit Bug Importer
Comment 11
2021-05-30 17:30:22 PDT
<
rdar://problem/78669976
>
WebKit Commit Bot
Comment 12
2021-05-31 18:14:31 PDT
Re-opened since this is blocked by
bug 226477
Sam Weinig
Comment 13
2021-05-31 18:41:18 PDT
Bizarre, I'm guessing "enum CSSValueID : uint16_t" is the issue, but why?
Sam Weinig
Comment 14
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.
Sam Weinig
Comment 15
2021-05-31 18:47:19 PDT
Created
attachment 430222
[details]
Patch
EWS
Comment 16
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug