XLinfini/sousa-gecko
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 2032954 - Make map_origin_color not eat error for unresolvable color. r=firefox-style-system-reviewers,layout-reviewers,gerard-majax,dshin

Browse Source 
See the test added for what's going on. Basically, we hit a color we
can't resolve like light-dark() from inside a relative color, and
map_origin_color will happily swallow that error, causing an assert
afterwards (because of a relative color with no origin color) instead of
the failure that's supposed to occur.

Differential Revision: https://phabricator.services.mozilla.com/D294934
This commit is contained in:
Emilio Cobos Álvarez
2026-04-20 10:07:28 +00:00
committed by ealvarez@mozilla.com
parent 174e4d4b8d
commit 2d64b11869
7 changed files with 119 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files
layout/inspector/tests/test_color_to_rgba.html
+56 -55
View File
@@ -1,60 +1,61 @@
<!DOCTYPE HTML>
<html>
<head>
<meta charset="utf-8">
<title>Test InspectorUtils::ColorToRGBA</title>
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
<script type="application/javascript">
const InspectorUtils = SpecialPowers.wrap(window).InspectorUtils;
testColor("red", {r:255, g:0, b:0, a:1});
testColor("#f00", {r:255, g:0, b:0, a:1});
testColor("#ff0000", {r:255, g:0, b:0, a:1});
testColor("ff0000", null);
testColor("rgb(255,0,0)", {r:255, g:0, b:0, a:1});
testColor("rgba(255,0,0)", {r:255, g:0, b:0, a:1});
testColor("rgb(255,0,0,0.7)", {r:255, g:0, b:0, a:0.7});
testColor("rgba(255,0,0,0.7)", {r:255, g:0, b:0, a:0.7});
testColor("rgb(50%,75%,60%)", {r:128, g:191, b:153, a:1});
testColor("rgba(50%,75%,60%)", {r:128, g:191, b:153, a:1});
testColor("rgb(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
testColor("rgba(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
testColor("hsl(320,30%,10%)", {r:33, g:18, b:28, a:1});
testColor("hsla(320,30%,10%)", {r:33, g:18, b:28, a:1});
testColor("hsl(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
testColor("hsla(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
isnot(
InspectorUtils.colorToRGBA("ButtonText"),
null,
"Should support system colors"
);
function testColor(color, expected) {
let rgb = InspectorUtils.colorToRGBA(color);
if (rgb === null) {
ok(expected === null, "color: " + color + " returns null");
return;
}
let {r, g, b, a} = rgb;
is(r, expected.r, "color: " + color + ", red component is converted correctly");
is(g, expected.g, "color: " + color + ", green component is converted correctly");
is(b, expected.b, "color: " + color + ", blue component is converted correctly");
is(Math.round(a * 10) / 10, expected.a, "color: " + color + ", alpha component is a converted correctly");
}
</script>
</head>
<body>
<meta charset="utf-8">
<title>Test InspectorUtils::ColorToRGBA</title>
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
<h1>Test InspectorUtils::ColorToRGBA</h1>
<iframe id="frame" style="display:none"></iframe>
<p id="display"></p>
<div id="content" style="display: none">
</div>
<pre id="test">
</pre>
</body>
</html>
<pre id="test"></pre>
<script>
const InspectorUtils = SpecialPowers.wrap(window).InspectorUtils;
function testColor(color, expected) {
let rgb = InspectorUtils.colorToRGBA(color);
if (rgb === null) {
ok(expected === null, "color: " + color + " returns null");
return;
}
let {r, g, b, a} = rgb;
is(r, expected.r, "color: " + color + ", red component is converted correctly");
is(g, expected.g, "color: " + color + ", green component is converted correctly");
is(b, expected.b, "color: " + color + ", blue component is converted correctly");
is(Math.round(a * 10) / 10, expected.a, "color: " + color + ", alpha component is a converted correctly");
}
testColor("red", {r:255, g:0, b:0, a:1});
testColor("#f00", {r:255, g:0, b:0, a:1});
testColor("#ff0000", {r:255, g:0, b:0, a:1});
testColor("ff0000", null);
testColor("rgb(255,0,0)", {r:255, g:0, b:0, a:1});
testColor("rgba(255,0,0)", {r:255, g:0, b:0, a:1});
testColor("rgb(255,0,0,0.7)", {r:255, g:0, b:0, a:0.7});
testColor("rgba(255,0,0,0.7)", {r:255, g:0, b:0, a:0.7});
testColor("rgb(50%,75%,60%)", {r:128, g:191, b:153, a:1});
testColor("rgba(50%,75%,60%)", {r:128, g:191, b:153, a:1});
testColor("rgb(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
testColor("rgba(100%,50%,25%,0.7)", {r:255, g:128, b:64, a:0.7});
testColor("hsl(320,30%,10%)", {r:33, g:18, b:28, a:1});
testColor("hsla(320,30%,10%)", {r:33, g:18, b:28, a:1});
testColor("hsl(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
testColor("hsla(170,60%,40%,0.9)", {r:41, g:163, b:143, a:0.9});
isnot(
InspectorUtils.colorToRGBA("ButtonText"),
null,
"Should support system colors"
);
let frameUtils =
SpecialPowers.wrap(document.getElementById("frame").contentWindow).InspectorUtils;
let lightDarkDependentRelativeColor = "oklch(from light-dark(oklch(55% 0.24 260), oklch(83% 0.17 260)) l c h / 20%)";
// If we ever make these work, switch to a sandbox / non-window global instead.
is(
frameUtils.colorToRGBA(lightDarkDependentRelativeColor),
null,
"Frame with no presShell shouldn't assert when resolving relative colors"
);
</script>
servo/components/style/color/color_function.rs
+22 -9
View File
@@ -371,18 +371,25 @@ impl ColorFunction<SpecifiedColor> {
/// contain any variables (currentcolor, color components, etc.).
pub fn resolve_to_absolute(&self) -> Result<AbsoluteColor, ()> {
// Map the color function to one with an absolute origin color.
let resolvable = self.map_origin_color(|o| o.resolve_to_absolute());
resolvable.resolve_to_absolute()
self.map_origin_color(|o| o.resolve_to_absolute())?
.resolve_to_absolute()
}
}
impl<Color> ColorFunction<Color> {
/// Map the origin color to another type. Return None from `f` if the conversion fails.
pub fn map_origin_color<U>(&self, f: impl FnOnce(&Color) -> Option<U>) -> ColorFunction<U> {
/// Map the origin color to another type.
pub fn map_origin_color<U>(
&self,
f: impl FnOnce(&Color) -> Result<U, ()>,
) -> Result<ColorFunction<U>, ()> {
macro_rules! map {
($f:ident, $o:expr, $c0:expr, $c1:expr, $c2:expr, $alpha:expr) => {{
ColorFunction::$f(
$o.as_ref().and_then(f).into(),
match $o.as_ref() {
Some(c) => Some(f(c)?),
None => None,
}
.into(),
$c0.clone(),
$c1.clone(),
$c2.clone(),
@@ -390,7 +397,7 @@ impl<Color> ColorFunction<Color> {
)
}};
}
match self {
Ok(match self {
ColorFunction::Rgb(o, c0, c1, c2, alpha) => map!(Rgb, o, c0, c1, c2, alpha),
ColorFunction::Hsl(o, c0, c1, c2, alpha) => map!(Hsl, o, c0, c1, c2, alpha),
ColorFunction::Hwb(o, c0, c1, c2, alpha) => map!(Hwb, o, c0, c1, c2, alpha),
@@ -399,14 +406,18 @@ impl<Color> ColorFunction<Color> {
ColorFunction::Oklab(o, c0, c1, c2, alpha) => map!(Oklab, o, c0, c1, c2, alpha),
ColorFunction::Oklch(o, c0, c1, c2, alpha) => map!(Oklch, o, c0, c1, c2, alpha),
ColorFunction::Color(o, c0, c1, c2, alpha, color_space) => ColorFunction::Color(
o.as_ref().and_then(f).into(),
match o.as_ref() {
Some(c) => Some(f(c)?),
None => None,
}
.into(),
c0.clone(),
c1.clone(),
c2.clone(),
alpha.clone(),
color_space.clone(),
),
}
})
}
}
@@ -414,7 +425,9 @@ impl ColorFunction<ComputedColor> {
/// Resolve a computed color function to an absolute computed color.
pub fn resolve_to_absolute(&self, current_color: &AbsoluteColor) -> AbsoluteColor {
// Map the color function to one with an absolute origin color.
let resolvable = self.map_origin_color(|o| Some(o.resolve_to_absolute(current_color)));
let resolvable = self
.map_origin_color(|o| Ok(o.resolve_to_absolute(current_color)))
.unwrap();
match resolvable.resolve_to_absolute() {
Ok(color) => color,
Err(..) => {
servo/components/style/color/parsing.rs
+3 -1
View File
@@ -146,7 +146,9 @@ fn parse_color_function<'i, 't>(
// Validate the channels and calc expressions by trying to resolve them against
// transparent.
// FIXME(emilio, bug 1925572): This could avoid cloning, or be done earlier.
let abs = color.map_origin_color(|_| Some(AbsoluteColor::TRANSPARENT_BLACK));
let abs = color
.map_origin_color(|_| Ok(AbsoluteColor::TRANSPARENT_BLACK))
.unwrap();
if abs.resolve_to_absolute().is_err() {
return Err(arguments.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}
servo/components/style/stylesheets/font_palette_values_rule.rs
+1 -1
View File
@@ -52,7 +52,7 @@ impl Parse for FontPaletteOverrideColor {
// https://drafts.csswg.org/css-color-5/#absolute-color
// so check that the specified color can be resolved without a context
// or currentColor value.
if color.resolve_to_absolute().is_some() {
if color.resolve_to_absolute().is_ok() {
// We store the specified color (not the resolved absolute color)
// because that is what the rule exposes to authors.
return Ok(FontPaletteOverrideColor { index, color });
servo/components/style/values/specified/color.rs
+31 -30
View File
@@ -619,7 +619,7 @@ impl Color {
context: &ParserContext,
input: &mut Parser,
device: Option<&Device>,
) -> Option<ComputedColor> {
) -> Result<ComputedColor, ()> {
use crate::error_reporting::ContextualParseError;
let start = input.position();
let result = input
@@ -628,22 +628,22 @@ impl Color {
let specified = match result {
Ok(s) => s,
Err(e) => {
if !context.error_reporting_enabled() {
return None;
if context.error_reporting_enabled() {
// Ignore other kinds of errors that might be reported, such as
// ParseErrorKind::Basic(BasicParseErrorKind::UnexpectedToken),
// since Gecko didn't use to report those to the error console.
//
// TODO(emilio): Revise whether we want to keep this at all, we
// use this only for canvas, this warnings are disabled by
// default and not available on OffscreenCanvas anyways...
if let ParseErrorKind::Custom(StyleParseErrorKind::ValueError(..)) = e.kind {
let location = e.location.clone();
let error =
ContextualParseError::UnsupportedValue(input.slice_from(start), e);
context.log_css_error(location, error);
}
}
// Ignore other kinds of errors that might be reported, such as
// ParseErrorKind::Basic(BasicParseErrorKind::UnexpectedToken),
// since Gecko didn't use to report those to the error console.
//
// TODO(emilio): Revise whether we want to keep this at all, we
// use this only for canvas, this warnings are disabled by
// default and not available on OffscreenCanvas anyways...
if let ParseErrorKind::Custom(StyleParseErrorKind::ValueError(..)) = e.kind {
let location = e.location.clone();
let error = ContextualParseError::UnsupportedValue(input.slice_from(start), e);
context.log_css_error(location, error);
}
return None;
return Err(());
},
};
@@ -733,12 +733,12 @@ impl Color {
/// forms that are invalid in an absolute color.
/// https://drafts.csswg.org/css-color-5/#absolute-color
/// Returns None if the specified color is not valid as an absolute color.
pub fn resolve_to_absolute(&self) -> Option<AbsoluteColor> {
pub fn resolve_to_absolute(&self) -> Result<AbsoluteColor, ()> {
use crate::values::specified::percentage::ToPercentage;
match self {
Self::Absolute(c) => Some(c.color),
Self::ColorFunction(ref color_function) => color_function.resolve_to_absolute().ok(),
Self::Absolute(c) => Ok(c.color),
Self::ColorFunction(ref color_function) => color_function.resolve_to_absolute(),
Self::ColorMix(ref mix) => {
use crate::color::mix;
@@ -750,9 +750,9 @@ impl Color {
))
}
Some(mix::mix_many(mix.interpolation, items, mix.flags))
Ok(mix::mix_many(mix.interpolation, items, mix.flags))
},
_ => None,
_ => Err(()),
}
}
@@ -855,7 +855,7 @@ impl Color {
///
/// If `context` is `None`, and the specified color requires data from
/// the context to resolve, then `None` is returned.
pub fn to_computed_color(&self, context: Option<&Context>) -> Option<ComputedColor> {
pub fn to_computed_color(&self, context: Option<&Context>) -> Result<ComputedColor, ()> {
macro_rules! adjust_absolute_color {
($color:expr) => {{
// Computed lightness values can not be NaN.
@@ -875,7 +875,7 @@ impl Color {
}};
}
Some(match *self {
Ok(match *self {
Color::CurrentColor => ComputedColor::CurrentColor,
Color::Absolute(ref absolute) => {
let mut color = absolute.color;
@@ -891,11 +891,11 @@ impl Color {
ComputedColor::Absolute(absolute)
} else {
let color_function = color_function
.map_origin_color(|origin_color| origin_color.to_computed_color(context));
.map_origin_color(|origin_color| origin_color.to_computed_color(context))?;
ComputedColor::ColorFunction(Box::new(color_function))
}
},
Color::LightDark(ref ld) => ld.compute(context?),
Color::LightDark(ref ld) => ld.compute(context.ok_or(())?),
Color::ColorMix(ref mix) => {
use crate::values::computed::percentage::Percentage;
@@ -916,9 +916,9 @@ impl Color {
Color::ContrastColor(ref c) => {
ComputedColor::ContrastColor(Box::new(c.to_computed_color(context)?))
},
Color::System(system) => system.compute(context?),
Color::System(system) => system.compute(context.ok_or(())?),
Color::InheritFromBodyQuirk => {
ComputedColor::Absolute(context?.device().body_text_color())
ComputedColor::Absolute(context.ok_or(())?.device().body_text_color())
},
})
}
@@ -928,7 +928,7 @@ impl ToComputedValue for Color {
type ComputedValue = ComputedColor;
fn to_computed_value(&self, context: &Context) -> ComputedColor {
self.to_computed_color(Some(context)).unwrap_or_else(|| {
self.to_computed_color(Some(context)).unwrap_or_else(|_| {
debug_assert!(
false,
"Specified color could not be resolved to a computed color!"
@@ -941,8 +941,9 @@ impl ToComputedValue for Color {
match *computed {
ComputedColor::Absolute(ref color) => Self::from_absolute_color(color.clone()),
ComputedColor::ColorFunction(ref color_function) => {
let color_function =
color_function.map_origin_color(|o| Some(Self::from_computed_value(o)));
let color_function = color_function
.map_origin_color(|o| Ok(Self::from_computed_value(o)))
.unwrap();
Self::ColorFunction(Box::new(color_function))
},
ComputedColor::CurrentColor => Color::CurrentColor,
servo/components/style/values/specified/effects.rs
+2 -6
View File
@@ -242,15 +242,11 @@ impl Filter {
))),
Filter::DropShadow(ref shadow) => {
if cfg!(feature = "gecko") {
let color = match shadow
let color = shadow
.color
.as_ref()
.unwrap_or(&Color::currentcolor())
.to_computed_color(None)
{
Some(c) => c,
None => return Err(()),
};
.to_computed_color(None)?;
let horizontal = ComputedCSSPixelLength::new(
shadow
servo/ports/geckolib/glue.rs
+4 -4
View File
@@ -9021,7 +9021,7 @@ unsafe fn compute_color(
current_color: &AbsoluteColor,
value: &nsACString,
loader: *mut Loader,
) -> Option<ComputeColorResult> {
) -> Result<ComputeColorResult, ()> {
let mut input = ParserInput::new(value.as_str_unchecked());
let mut input = Parser::new(&mut input);
let reporter = loader.as_mut().and_then(|loader| {
@@ -9054,7 +9054,7 @@ unsafe fn compute_color(
let result_color = computed.resolve_to_absolute(current_color);
let was_current_color = computed.is_currentcolor();
Some(ComputeColorResult {
Ok(ComputeColorResult {
result_color,
was_current_color,
})
@@ -9070,7 +9070,7 @@ pub unsafe extern "C" fn Servo_ComputeColor(
loader: *mut Loader,
) -> bool {
let current_color = AbsoluteColor::from_nscolor(current_color);
let Some(result) = compute_color(raw_data, &current_color, value, loader) else {
let Ok(result) = compute_color(raw_data, &current_color, value, loader) else {
return false;
};
@@ -9087,7 +9087,7 @@ pub unsafe extern "C" fn Servo_ComputeAbsoluteColor(
value: &nsACString,
result_color: &mut AbsoluteColor,
) -> bool {
if let Some(color) = compute_color(raw_data, &AbsoluteColor::BLACK, value, ptr::null_mut()) {
if let Ok(color) = compute_color(raw_data, &AbsoluteColor::BLACK, value, ptr::null_mut()) {
*result_color = color.result_color;
true
} else {