Skip to content

Commit

Permalink
Clean up code for internal CSS values
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=267520
rdar://121019340

Reviewed by Darin Adler.

Right now, if you want to make a certain value internal, you have to do 2 changes:
1) Add "status": "internal" to CSSProperties.json
2) Change isValueAllowedInMode() to include your value

This is error prone, as it's easy to think that the first step is sufficient.

With this change:
- the first step will be sufficient to make a value UA-sheet only
- isValueAllowedInMode is now named isColorKeywordAllowedInMode as it's now only used for color parsing.
- we now validate that all values starting with "-internal-" have "status": "internal" in CSSProperties.json

Also use isUASheetBehavior whenever possible.

* Source/WebCore/css/parser/CSSParserContext.cpp:
(WebCore::CSSParserContext::CSSParserContext):
* Source/WebCore/css/parser/CSSParserFastPaths.cpp:
(WebCore::parseColor):
* Source/WebCore/css/parser/CSSParserIdioms.cpp:
(WebCore::isColorKeywordAllowedInMode):
(WebCore::isValueAllowedInMode): Deleted.
* Source/WebCore/css/parser/CSSParserIdioms.h:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::LengthRawKnownTokenTypeDimensionConsumer::consume):
(WebCore::CSSPropertyParserHelpers::consumeColorWorkerSafe):
(WebCore::CSSPropertyParserHelpers::consumeColor):
(WebCore::CSSPropertyParserHelpers::consumeCounterStyleNameInPrelude):
(WebCore::CSSPropertyParserHelpers::consumeDisplay):
* Source/WebCore/css/parser/CSSSelectorParser.cpp:
(WebCore::extractCompoundFlags):
(WebCore::CSSSelectorParser::consumeSimpleSelector):
(WebCore::CSSSelectorParser::splitCompoundAtImplicitShadowCrossingCombinator):
* Source/WebCore/css/process-css-properties.py:
Directly check for UASheetMode and add validation for `-internal-` values that they use correct "status" field.

* Source/WebCore/css/process-css-pseudo-selectors.py:
The .join() case also works fine for when there's only one condition.

Canonical link: https://commits.webkit.org/273071@main
  • Loading branch information
nt1m committed Jan 16, 2024
1 parent e213ae7 commit 8c8cfbf
Show file tree
Hide file tree
Showing 8 changed files with 18 additions and 21 deletions.
2 changes: 1 addition & 1 deletion Source/WebCore/css/parser/CSSParserContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ CSSParserContext::CSSParserContext(CSSParserMode mode, const URL& baseURL)
, mode(mode)
{
// FIXME: We should turn all of the features on from their WebCore Settings defaults.
if (mode == UASheetMode) {
if (isUASheetBehavior(mode)) {
colorMixEnabled = true;
focusVisibleEnabled = true;
lightDarkEnabled = true;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/css/parser/CSSParserFastPaths.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ static RefPtr<CSSValue> parseColor(StringView string, const CSSParserContext& co
ASSERT(!string.isEmpty());
auto valueID = cssValueKeywordID(string);
if (StyleColor::isColorKeyword(valueID)) {
if (!isValueAllowedInMode(valueID, context.mode))
if (!isColorKeywordAllowedInMode(valueID, context.mode))
return nullptr;
return CSSPrimitiveValue::create(valueID);
}
Expand Down
4 changes: 1 addition & 3 deletions Source/WebCore/css/parser/CSSParserIdioms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

namespace WebCore {

bool isValueAllowedInMode(unsigned short id, CSSParserMode mode)
bool isColorKeywordAllowedInMode(CSSValueID id, CSSParserMode mode)
{
switch (id) {
case CSSValueWebkitFocusRingColor:
Expand All @@ -46,8 +46,6 @@ bool isValueAllowedInMode(unsigned short id, CSSParserMode mode)
case CSSValueAppleSystemQuaternaryFill:
#endif
case CSSValueInternalDocumentTextColor:
case CSSValueInternalTextareaAuto:
case CSSValueInternalThCenter:
return isUASheetBehavior(mode);
default:
return true;
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/css/parser/CSSParserIdioms.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ bool isNameCodePoint(CharacterType c)
return isNameStartCodePoint(c) || isASCIIDigit(c) || c == '-';
}

bool isValueAllowedInMode(unsigned short, CSSParserMode);
bool isColorKeywordAllowedInMode(CSSValueID, CSSParserMode);

inline bool isCSSWideKeyword(CSSValueID valueID)
{
Expand Down
10 changes: 5 additions & 5 deletions Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ struct LengthRawKnownTokenTypeDimensionConsumer {
auto unitType = token.unitType();
switch (unitType) {
case CSSUnitType::CSS_QUIRKY_EM:
if (parserMode != UASheetMode)
if (!isUASheetBehavior(parserMode))
return std::nullopt;
FALLTHROUGH;
case CSSUnitType::CSS_EM:
Expand Down Expand Up @@ -3310,7 +3310,7 @@ Color consumeColorWorkerSafe(CSSParserTokenRange& range, const CSSParserContext&
// For now, we detect the system color, but then intentionally fail parsing.
if (StyleColor::isSystemColorKeyword(keyword))
return { };
if (!isValueAllowedInMode(keyword, context.mode))
if (!isColorKeywordAllowedInMode(keyword, context.mode))
return { };
result = StyleColor::colorFromKeyword(keyword, { });
range.consumeIncludingWhitespace();
Expand All @@ -3331,7 +3331,7 @@ RefPtr<CSSPrimitiveValue> consumeColor(CSSParserTokenRange& range, const CSSPars
{
auto keyword = range.peek().id();
if (StyleColor::isColorKeyword(keyword, allowedColorTypes)) {
if (!isValueAllowedInMode(keyword, context.mode))
if (!isColorKeywordAllowedInMode(keyword, context.mode))
return nullptr;
return consumeIdent(range);
}
Expand Down Expand Up @@ -4811,7 +4811,7 @@ AtomString consumeCounterStyleNameInPrelude(CSSParserTokenRange& prelude, CSSPar
// case-insensitive match for "decimal", "disc", "square", "circle", "disclosure-open" and "disclosure-closed". No <counter-style-name>, prelude or not, may be an ASCII
// case-insensitive match for "none".
auto id = nameToken.id();
if (identMatches<CSSValueNone>(id) || (mode != CSSParserMode::UASheetMode && identMatches<CSSValueDecimal, CSSValueDisc, CSSValueCircle, CSSValueSquare, CSSValueDisclosureOpen, CSSValueDisclosureClosed>(id)))
if (identMatches<CSSValueNone>(id) || (!isUASheetBehavior(mode) && identMatches<CSSValueDecimal, CSSValueDisc, CSSValueCircle, CSSValueSquare, CSSValueDisclosureOpen, CSSValueDisclosureClosed>(id)))
return AtomString();
auto name = nameToken.value();
return isPredefinedCounterStyle(nameToken.id()) ? name.convertToASCIILowercaseAtom() : name.toAtomString();
Expand Down Expand Up @@ -5159,7 +5159,7 @@ RefPtr<CSSValue> consumeDisplay(CSSParserTokenRange& range, CSSParserMode mode)

auto allowsValue = [&](CSSValueID value) {
bool isRuby = value == CSSValueRubyBase || value == CSSValueRubyText || value == CSSValueBlockRuby || value == CSSValueRuby;
return !isRuby || mode == CSSParserMode::UASheetMode;
return !isRuby || isUASheetBehavior(mode);
};

if (singleKeyword) {
Expand Down
6 changes: 3 additions & 3 deletions Source/WebCore/css/parser/CSSSelectorParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ static OptionSet<CompoundSelectorFlag> extractCompoundFlags(const MutableCSSSele
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=161747
// The UASheetMode check is a work-around to allow this selector in mediaControls(New).css:
// input[type="range" i]::-webkit-media-slider-container > div {
if (parserMode == UASheetMode && simpleSelector.pseudoElement() == CSSSelector::PseudoElement::UserAgentPart)
if (isUASheetBehavior(parserMode) && simpleSelector.pseudoElement() == CSSSelector::PseudoElement::UserAgentPart)
return { };

return CompoundSelectorFlag::HasPseudoElementForRightmostCompound;
Expand Down Expand Up @@ -575,7 +575,7 @@ std::unique_ptr<MutableCSSSelector> CSSSelectorParser::consumeSimpleSelector(CSS
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=161747
// The UASheetMode check is a work-around to allow this selector in mediaControls(New).css:
// video::-webkit-media-text-track-region-container.scrolling
if (m_context.mode != UASheetMode && !isSimpleSelectorValidAfterPseudoElement(*selector, *m_precedingPseudoElement))
if (!isUASheetBehavior(m_context.mode) && !isSimpleSelectorValidAfterPseudoElement(*selector, *m_precedingPseudoElement))
m_failedParsing = true;
}

Expand Down Expand Up @@ -1157,7 +1157,7 @@ std::unique_ptr<MutableCSSSelector> CSSSelectorParser::splitCompoundAtImplicitSh
bool isSlotted = splitAfter->tagHistory()->match() == CSSSelector::Match::PseudoElement && splitAfter->tagHistory()->pseudoElement() == CSSSelector::PseudoElement::Slotted;

std::unique_ptr<MutableCSSSelector> secondCompound;
if (context.mode == UASheetMode || isPart) {
if (isUASheetBehavior(context.mode) || isPart) {
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=161747
// We have to recur, since we have rules in media controls like video::a::b. This should not be allowed, and
// we should remove this recursion once those rules are gone.
Expand Down
7 changes: 5 additions & 2 deletions Source/WebCore/css/process-css-properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ def from_json(parsing_context, key_path, json_value):
print(f"SKIPPED value {json_value['value']} in {key_path} due to '{json_value['status']}' status designation.")
return None

if json_value.get("status", None) != "internal" and json_value["value"].startswith("-internal-"):
raise Exception(f'Value "{json_value["value"]}" starts with "-internal-" but does not have "status": "internal" set.')

return Value(**json_value)

@property
Expand Down Expand Up @@ -4514,7 +4517,7 @@ def _generate(self, *, to, range_string, context_string, default_string):
else:
conditions.append(f"!{context_string}.{keyword_term.settings_flag}")
if keyword_term.status == "internal":
conditions.append(f"!isValueAllowedInMode(keyword, {context_string}.mode)")
conditions.append(f"!isUASheetBehavior({context_string}.mode)")

if keyword_term.aliased_to:
return_value = keyword_term.aliased_to.id
Expand Down Expand Up @@ -4623,7 +4626,7 @@ def generate_definition(self, *, to):
else:
return_expression.append(f"context.{keyword_term.settings_flag}")
if keyword_term.status == "internal":
return_expression.append("isValueAllowedInMode(keyword, context.mode)")
return_expression.append("isUASheetBehavior(context.mode)")

keyword_term_and_return_expressions.append(KeywordTermReturnExpression(keyword_term, return_expression))

Expand Down
6 changes: 1 addition & 5 deletions Source/WebCore/css/process-css-pseudo-selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -637,11 +637,7 @@ def format_enablement_condition(self, settings_flag, is_internal):
conditions.append(f'context.{settings_flag}')

if is_internal:
# Wrap around parentheses so it's valid when negated with `!boolean`.
conditions.append('(context.mode == UASheetMode)')

if len(conditions) == 1:
return conditions[0]
conditions.append('isUASheetBehavior(context.mode)')

return ' && '.join(conditions)

Expand Down

0 comments on commit 8c8cfbf

Please sign in to comment.