Test case: https://jsfiddle.net/ericwilligers/2q9cmrha/ Spec: https://drafts.csswg.org/css-fonts-3/#propdef-font-synthesis none | [ weight || style ] If the values 'weight style' or 'style weight' are assigned, the computed style should be 'weight style', but WebKit gives 'style weight'.
I thought the spec was in the process of changing the syntax fairly dramatically? I mean, we can flip the order super easily, but if we're going to be changing our implementation substantially, we may want to wait.
Looks like we're gaining interoperability on that ordering, so I guess we should: https://wpt.fyi/results/css/css-fonts/parsing/font-synthesis-valid.html?label=master&label=experimental&product=chrome&product=firefox&product=webkitgtk&product=safari&aligned
Created attachment 443153 [details] Patch
Comment on attachment 443153 [details] Patch Layout test results will be updated with the next patchset.
Comment on attachment 443153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443153&action=review r=mews > Source/WebCore/css/StyleProperties.cpp:173 > + case CSSPropertyFontSynthesis: > + return fontSynthesisPropertyValue(*value); This is a surprising place for this code. Why is this here and not below (or maybe even in another function?)
Comment on attachment 443153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443153&action=review >> Source/WebCore/css/StyleProperties.cpp:173 >> + return fontSynthesisPropertyValue(*value); > > This is a surprising place for this code. Why is this here and not below (or maybe even in another function?) Falling through from CSSPropertyStrokeOpacity and returning a fontSynthesisPropertyValue() doesn't seem right.
Comment on attachment 443153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443153&action=review >>> Source/WebCore/css/StyleProperties.cpp:173 >>> + return fontSynthesisPropertyValue(*value); >> >> This is a surprising place for this code. Why is this here and not below (or maybe even in another function?) > > Falling through from CSSPropertyStrokeOpacity and returning a fontSynthesisPropertyValue() doesn't seem right. I moved this callsite to above. Originally I placed this here because it was right before default switch but I didn't see the FALLTHROUGH. And it seems that below place is for shorthand properties, so I will place this call as below. if (auto value = getPropertyCSSValue(propertyID)) { switch (propertyID) { case CSSPropertyFontSynthesis: return fontSynthesisPropertyValue(*value); case CSSPropertyFillOpacity: case CSSPropertyFloodOpacity: ...
Created attachment 443174 [details] Patch
I updated layout test result on Ubuntu 21.10 and will update layout test result on macOS 12.0.1 too in the next patchset.
Created attachment 443195 [details] Patch
Could you please review this patch again? I think this patch is ready for review now.
Created attachment 443281 [details] Update layout test results for ios and common
Hi, please review this patch when you are available. It seems that ews has passed.
Comment on attachment 443281 [details] Update layout test results for ios and common View in context: https://bugs.webkit.org/attachment.cgi?id=443281&action=review > Source/WebCore/css/StyleProperties.cpp:505 > +String StyleProperties::fontSynthesisPropertyValue(CSSValue& value) const > +{ > + String valueStr = value.cssText(); > + if (isCSSWideValueKeyword(valueStr) || is<CSSPrimitiveValue>(value)) > + return valueStr; > + > + if (is<CSSValueList>(value)) { > + bool hasWeight = false; > + bool hasStyle = false; > + if (downcast<CSSValueList>(value).hasValue(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight).ptr())) > + hasWeight = true; > + if (downcast<CSSValueList>(value).hasValue(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle).ptr())) > + hasStyle = true; > + return makeString(hasWeight ? "weight" : "", (hasWeight && hasStyle) ? " " : "", hasStyle ? "style" : ""); > + } > + > + return String(); > +} I think, instead of writing this function, we should change consumeFontSynthesis() in CSSPropertyParser.cpp to just canonicalize the list at parsing time. Canonicalizing at parse time would be a simpler solution, and there would not have to be another special case here - StyleProperties::getPropertyValue() would be entirely unmodified. Maybe something like this: bool foundWeight = false; bool foundStyle = false; bool foundSmallCaps = false; while (true) { auto ident = consumeIdent<CSSValueWeight, CSSValueStyle, CSSValueSmallCaps>(range); if (!ident) break; switch (ident->valueID()) { case CSSValueWeight: if (foundWeight) return nullptr; foundWeight = true; case CSSValueStyle: if (foundStyle) return nullptr; foundStyle = true; case CSSValueSmallCaps: if (foundSmallCaps) return nullptr; foundSmallCaps = true; } } RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); if (foundWeight) list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight)); if (foundStyle) list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle)); if (foundSmallCaps) list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueSmallCaps)); if (!list->length()) return nullptr; return list; It's probably even possible to make this more concise by using lambdas.
Comment on attachment 443281 [details] Update layout test results for ios and common View in context: https://bugs.webkit.org/attachment.cgi?id=443281&action=review >> Source/WebCore/css/StyleProperties.cpp:505 >> +} > > I think, instead of writing this function, we should change consumeFontSynthesis() in CSSPropertyParser.cpp to just canonicalize the list at parsing time. Canonicalizing at parse time would be a simpler solution, and there would not have to be another special case here - StyleProperties::getPropertyValue() would be entirely unmodified. > > Maybe something like this: > > bool foundWeight = false; > bool foundStyle = false; > bool foundSmallCaps = false; > while (true) { > auto ident = consumeIdent<CSSValueWeight, CSSValueStyle, CSSValueSmallCaps>(range); > if (!ident) > break; > switch (ident->valueID()) { > case CSSValueWeight: > if (foundWeight) > return nullptr; > foundWeight = true; > case CSSValueStyle: > if (foundStyle) > return nullptr; > foundStyle = true; > case CSSValueSmallCaps: > if (foundSmallCaps) > return nullptr; > foundSmallCaps = true; > } > } > > RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > if (foundWeight) > list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight)); > if (foundStyle) > list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle)); > if (foundSmallCaps) > list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueSmallCaps)); > > if (!list->length()) > return nullptr; > return list; > > It's probably even possible to make this more concise by using lambdas. Thank you for your review, Myles:) I applied your comment in the next patchset and will upload it soon.
Created attachment 443477 [details] Canonicalize CSSValueList at parsing stage in consumeFontSynthesis
Comment on attachment 443477 [details] Canonicalize CSSValueList at parsing stage in consumeFontSynthesis View in context: https://bugs.webkit.org/attachment.cgi?id=443477&action=review > Source/WebCore/css/parser/CSSPropertyParser.cpp:995 > + auto makeCanonicalizedList = [](CSSParserTokenRange& range) -> RefPtr<CSSValueList> { The fact that this code is in a lambda doesn't seem to be useful. When I mentioned "lambda" I was suggesting using it as a tool to reduce (nearly-)duplicated code. If it's not possible to reduce (nearly-)duplicated code, I suggest we promote this code to no longer be within a lambda.
Comment on attachment 443477 [details] Canonicalize CSSValueList at parsing stage in consumeFontSynthesis View in context: https://bugs.webkit.org/attachment.cgi?id=443477&action=review >> Source/WebCore/css/parser/CSSPropertyParser.cpp:995 >> + auto makeCanonicalizedList = [](CSSParserTokenRange& range) -> RefPtr<CSSValueList> { > > The fact that this code is in a lambda doesn't seem to be useful. When I mentioned "lambda" I was suggesting using it as a tool to reduce (nearly-)duplicated code. > > If it's not possible to reduce (nearly-)duplicated code, I suggest we promote this code to no longer be within a lambda. I added a small tiny lambda below, except that I promoted all the codes to no longer be within a lambda in the next patchset. Thank you for your review. auto checkAndMarkExistence = [](bool* found) { if (*found) return false; return *found = true; };
Created attachment 443482 [details] Patch for landing
Committed r285383 (243940@main): <https://commits.webkit.org/243940@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443482 [details].
<rdar://problem/85105405>
Committed r285384 (243941@main): <https://commits.webkit.org/243941@main>