Skip to content

Commit

Permalink
ControlPart enum order should not depend on CSSValueID enum order
Browse files Browse the repository at this point in the history
Currently, the order of ControlPart enum in theme_types.h must match the
order in css_value_keywords.json5. If not, a developer who uses
an appearance value will get an unexpected rendered result when the
wrong ControlPart value gets mapped instead. That is happening in M118
since CL [1] moved the order in css_value_keywords.json5, but not in
theme_types.h.

We fix this bug by changing the function `inline ControlPart
CSSIdentifierValue::ConvertTo()` to not depend on the enum value order
and instead list all ControlParts in a switch case.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/4833813

Change-Id: Ib61346281ee2a3de07c80729dc3029c97d3aee55
Bug: 1495418, 924486
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4973285
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215572}
  • Loading branch information
dizhang168 authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent a110627 commit c9694e8
Show file tree
Hide file tree
Showing 13 changed files with 101 additions and 15 deletions.
63 changes: 56 additions & 7 deletions third_party/blink/renderer/core/css/css_primitive_value_mappings.h
Original file line number Diff line number Diff line change
Expand Up @@ -330,14 +330,63 @@ inline CSSIdentifierValue::CSSIdentifierValue(ControlPart e)

template <>
inline ControlPart CSSIdentifierValue::ConvertTo() const {
if (value_id_ == CSSValueID::kNone) {
return kNoControlPart;
}
if (value_id_ == CSSValueID::kAuto) {
return kAutoPart;
switch (value_id_) {
case CSSValueID::kNone:
return kNoControlPart;
case CSSValueID::kAuto:
return kAutoPart;
case CSSValueID::kCheckbox:
return kCheckboxPart;
case CSSValueID::kRadio:
return kRadioPart;
case CSSValueID::kPushButton:
return kPushButtonPart;
case CSSValueID::kSquareButton:
return kSquareButtonPart;
case CSSValueID::kButton:
return kButtonPart;
case CSSValueID::kInnerSpinButton:
return kInnerSpinButtonPart;
case CSSValueID::kListbox:
return kListboxPart;
case CSSValueID::kMediaSlider:
return kMediaSliderPart;
case CSSValueID::kMediaSliderthumb:
return kMediaSliderThumbPart;
case CSSValueID::kMediaVolumeSlider:
return kMediaVolumeSliderPart;
case CSSValueID::kMediaVolumeSliderthumb:
return kMediaVolumeSliderThumbPart;
case CSSValueID::kInternalMediaControl:
return kMediaControlPart;
case CSSValueID::kMenulist:
return kMenulistPart;
case CSSValueID::kMenulistButton:
return kMenulistButtonPart;
case CSSValueID::kMeter:
return kMeterPart;
case CSSValueID::kProgressBar:
return kProgressBarPart;
case CSSValueID::kSliderHorizontal:
return kSliderHorizontalPart;
case CSSValueID::kSliderVertical:
return kSliderVerticalPart;
case CSSValueID::kSliderthumbHorizontal:
return kSliderThumbHorizontalPart;
case CSSValueID::kSliderthumbVertical:
return kSliderThumbVerticalPart;
case CSSValueID::kSearchfield:
return kSearchFieldPart;
case CSSValueID::kSearchfieldCancelButton:
return kSearchFieldCancelButtonPart;
case CSSValueID::kTextfield:
return kTextFieldPart;
case CSSValueID::kTextarea:
return kTextAreaPart;
default:
NOTREACHED();
return kNoControlPart;
}
return ControlPart(static_cast<int>(value_id_) -
static_cast<int>(CSSValueID::kCheckbox) + kCheckboxPart);
}

template <>
Expand Down
5 changes: 0 additions & 5 deletions third_party/blink/renderer/core/css/css_value_keywords.json5
Original file line number Diff line number Diff line change
Expand Up @@ -841,11 +841,6 @@
"manual",

// -webkit-appearance
// The order here must match the order in the ControlPart enum in
// theme_types.h.
// All appearance values that should be accepted by the parser should be
// listed between 'checkbox' and 'textarea'. This is checked in
// CSSParserFastPaths::IsValidKeywordPropertyAndValue.
"checkbox",
"radio",
"button",
Expand Down
3 changes: 0 additions & 3 deletions third_party/blink/renderer/platform/theme_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@

namespace blink {

// Must follow css_value_keywords.json5 order
// kAutoPart is never returned by ComputedStyle::EffectiveAppearance()
enum ControlPart {
kNoControlPart,
Expand All @@ -45,8 +44,6 @@ enum ControlPart {
kSearchFieldPart,
kTextFieldPart,
kTextAreaPart,
// Order matters when determinating what keyword is valid in the CSSParser.
// Values after kTextAreaPart should not be recognized as appearance values.
kInnerSpinButtonPart,
kMediaSliderPart,
kMediaSliderThumbPart,
Expand Down
1 change: 1 addition & 0 deletions third_party/blink/web_tests/VirtualTestSuites
Original file line number Diff line number Diff line change
Expand Up @@ -2471,6 +2471,7 @@
"prefix": "deprecated-css-appearance-values-enabled",
"platforms": ["Linux", "Mac", "Win"],
"bases": [
"css-parser/deprecated-css-appearance-values.html",
"css-parser/deprecated-css-appearance-values-warning/appearance-warning-cssom-usecounter.html",
"css-parser/deprecated-css-appearance-values-warning/appearance-warning-usecounter.html",
"css-parser/deprecated-css-appearance-values-warning/webkit-appearance-warning-cssom-usecounter.html",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CONSOLE WARNING: The keyword 'inner-spin-button' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'push-button' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'square-button' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'slider-horizontal' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'searchfield-cancel-button' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'media-slider' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'media-sliderthumb' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'media-volume-slider' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'media-volume-sliderthumb' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'sliderthumb-horizontal' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
CONSOLE WARNING: The keyword 'sliderthumb-vertical' used on the 'appearance' property was deprecated and has now been removed. It will no longer have any effect.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<!DOCTYPE html>
<title>Deprecated CSS appearance value: inner-spin-button</title>
<link rel=author href="mailto:dizhangg@chromium.org">
<link rel="help" href="https://drafts.csswg.org/css-ui-4/#appearance-switching">
<style>
span {
border: 2px solid;
border-color: red;
height: 3em;
}
</style>
<span style="appearance: inner-spin-button">foobar</span>
<span style="appearance: push-button">foobar</span>
<span style="appearance: square-button">foobar</span>
<span style="appearance: slider-horizontal">foobar</span>
<span style="appearance: searchfield-cancel-button">foobar</span>
<span style="appearance: media-slider">foobar</span>
<span style="appearance: media-sliderthumb">foobar</span>
<span style="appearance: media-volume-slider">foobar</span>
<span style="appearance: media-volume-sliderthumb">foobar</span>
<span style="appearance: sliderthumb-horizontal">foobar</span>
<span style="appearance: sliderthumb-vertical">foobar</span>
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
CONSOLE WARNING: The keyword 'inner-spin-button' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'push-button' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'square-button' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'slider-horizontal' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'searchfield-cancel-button' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'media-slider' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'media-sliderthumb' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'media-volume-slider' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'media-volume-sliderthumb' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'sliderthumb-horizontal' specified to an 'appearance' property is not standardized. It will be removed in the future.
CONSOLE WARNING: The keyword 'sliderthumb-vertical' specified to an 'appearance' property is not standardized. It will be removed in the future.

0 comments on commit c9694e8

Please sign in to comment.