From dc50a8e8b061fe88856a26e0a035ebf37488c5ab Mon Sep 17 00:00:00 2001 From: Kexuan Yang Date: Fri, 30 Oct 2026 11:10:03 +0800 Subject: [PATCH] Read bgcolor, font color and body text as legacy colors `bgcolor` only took `#rgb` and `#rrggbb`, and read `#rgb` without scaling each digit (`#fff` as `#0f0f0f`), so the white tables common in email painted near black. Color names, hex digits without the `#` and values with spaces around them were ignored. Parse them with Stylo's implementation of the HTML "rules for parsing a legacy colour value", as the dimension attributes already use Stylo's parser, and map `font color` and `body text` to `color`, which were not mapped at all. Co-Authored-By: Claude Opus 5.5 --- packages/blitz-dom/src/stylo.rs | 50 ++++----- tests/blitz-tests/tests/all.rs | 1 + .../tests/legacy_color_attributes.rs | 102 ++++++++++++++++++ 3 files changed, 126 insertions(+), 27 deletions(-) create mode 100644 tests/blitz-tests/tests/legacy_color_attributes.rs diff --git a/packages/blitz-dom/src/stylo.rs b/packages/blitz-dom/src/stylo.rs index 30881e5860..790bfd8283 100644 --- a/packages/blitz-dom/src/stylo.rs +++ b/packages/blitz-dom/src/stylo.rs @@ -23,7 +23,6 @@ use style::animation::AnimationSetKey; use style::animation::AnimationState; use style::applicable_declarations::ApplicableDeclarationBlock; use style::bloom::each_relevant_element_hash; -use style::color::AbsoluteColor; use style::data::{ElementDataMut, ElementDataRef}; use style::global_style_data::STYLE_THREAD_POOL; @@ -941,27 +940,15 @@ impl<'a> TElement for BlitzNode<'a> { )); }; - fn parse_color_attr(value: &str) -> Option<(u8, u8, u8, f32)> { - if !value.starts_with('#') { - return None; - } - - let value = &value[1..]; - if value.len() == 3 { - let r = u8::from_str_radix(&value[0..1], 16).ok()?; - let g = u8::from_str_radix(&value[1..2], 16).ok()?; - let b = u8::from_str_radix(&value[2..3], 16).ok()?; - return Some((r, g, b, 1.0)); - } - - if value.len() == 6 { - let r = u8::from_str_radix(&value[0..2], 16).ok()?; - let g = u8::from_str_radix(&value[2..4], 16).ok()?; - let b = u8::from_str_radix(&value[4..6], 16).ok()?; - return Some((r, g, b, 1.0)); - } - - None + /// The HTML "rules for parsing a legacy colour value" -- Stylo's + /// implementation of them. They take more than CSS does: color + /// names, `#rgb` as `#rrggbb`, hex digits without the `#`, spaces + /// around the value, and any other text read digit by digit. + /// + /// https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-a-legacy-colour-value + fn parse_color_attr(value: &str) -> Option { + let color = style::servo::attr::parse_legacy_color(value).ok()?; + Some(style::values::specified::Color::from_absolute_color(color)) } /// The HTML "rules for parsing dimension values" -- Stylo's @@ -1230,11 +1217,20 @@ impl<'a> TElement for BlitzNode<'a> { } if *name == local_name!("bgcolor") { - use style::values::specified::Color; - if let Some((r, g, b, a)) = parse_color_attr(value) { - push_style(PropertyDeclaration::BackgroundColor( - Color::from_absolute_color(AbsoluteColor::srgb_legacy(r, g, b, a)), - )); + if let Some(color) = parse_color_attr(value) { + push_style(PropertyDeclaration::BackgroundColor(color)); + } + } + + // `font color` and `body text` set the text color: + // https://html.spec.whatwg.org/multipage/rendering.html#phrasing-content-3 + // https://html.spec.whatwg.org/multipage/rendering.html#the-page + let sets_text_color = (*tag == local_name!("font") && *name == local_name!("color")) + || (*tag == local_name!("body") && *name == local_name!("text")); + if sets_text_color { + if let Some(color) = parse_color_attr(value) { + use style::values::specified::ColorPropertyValue; + push_style(PropertyDeclaration::Color(ColorPropertyValue(color))); } } diff --git a/tests/blitz-tests/tests/all.rs b/tests/blitz-tests/tests/all.rs index b3f5f639a1..094581e883 100644 --- a/tests/blitz-tests/tests/all.rs +++ b/tests/blitz-tests/tests/all.rs @@ -42,6 +42,7 @@ mod inner_html_leak; mod interaction_state_canonicalization; mod interaction_state_teardown; mod lang_attribute; +mod legacy_color_attributes; mod line_break; mod link_rel_attribute; mod oof_dynamic_cb; diff --git a/tests/blitz-tests/tests/legacy_color_attributes.rs b/tests/blitz-tests/tests/legacy_color_attributes.rs new file mode 100644 index 0000000000..dc5414edf0 --- /dev/null +++ b/tests/blitz-tests/tests/legacy_color_attributes.rs @@ -0,0 +1,102 @@ +//! `bgcolor`, `font color` and `body text` are read with the HTML "rules for +//! parsing a legacy colour value", which take more than CSS colors do. +//! +//! The engine only read `#rgb` and `#rrggbb`, and read `#rgb` wrong (`#fff` +//! as `#0f0f0f`), so the white tables of many emails came out near black. +//! `font color` and `body text` were not mapped at all. + +use blitz_dom::CopyStyleGetters; +use blitz_test_harness::Harness; +use style::color::AbsoluteColor; + +fn background_of(harness: &Harness, selector: &str) -> AbsoluteColor { + let id = harness.node(selector); + let doc = harness.base(); + let styles = doc.get_node(id).unwrap().primary_styles().unwrap(); + let current_color = styles.clone_color(); + (styles.get_background().background_color).resolve_to_absolute(¤t_color) +} + +fn color_of(harness: &Harness, selector: &str) -> AbsoluteColor { + let id = harness.node(selector); + let doc = harness.base(); + let styles = doc.get_node(id).unwrap().primary_styles().unwrap(); + styles.clone_color() +} + +/// An element with `attributes`, and one styled with `css` to compare it to. +fn page(tag: &str, attributes: &str, css: &str) -> Harness { + Harness::from_html(&format!( + "\ + <{tag} id=attr {attributes}>text\ + <{tag} id=css style='{css}'>text\ + " + )) +} + +#[test] +fn bgcolor_takes_legacy_color_values() { + for (value, css) in [ + ("#ff0000", "#ff0000"), + ("#fff", "#ffffff"), + ("#F00", "#ff0000"), + ("red", "red"), + ("LightGray", "lightgray"), + ("ff0000", "#ff0000"), + (" #ff0000 ", "#ff0000"), + // Any other text is read digit by digit. + ("chucknorris", "#c00000"), + ] { + let harness = page( + "div", + &format!("bgcolor='{value}'"), + &format!("background-color: {css}"), + ); + assert_eq!( + background_of(&harness, "#attr"), + background_of(&harness, "#css"), + "bgcolor={value:?}" + ); + } +} + +#[test] +fn bgcolor_that_is_no_color_sets_no_background() { + for value in ["", "transparent"] { + let harness = page("div", &format!("bgcolor='{value}'"), ""); + assert_eq!( + background_of(&harness, "#attr"), + background_of(&harness, "#css"), + "bgcolor={value:?}" + ); + } +} + +#[test] +fn font_color_and_body_text_set_the_text_color() { + for (value, css) in [("#fff", "#ffffff"), ("red", "red"), ("336699", "#336699")] { + let harness = page( + "font", + &format!("color='{value}'"), + &format!("color: {css}"), + ); + assert_eq!( + color_of(&harness, "#attr"), + color_of(&harness, "#css"), + "color={value:?}" + ); + } + + let harness = Harness::from_html( + "

text

\ +

text

", + ); + assert_eq!(color_of(&harness, "#p"), color_of(&harness, "#css")); +} + +/// `color` is only mapped on `font`. +#[test] +fn color_on_other_elements_is_ignored() { + let harness = page("p", "color=red", ""); + assert_eq!(color_of(&harness, "#attr"), color_of(&harness, "#css")); +}