Read bgcolor, font color and body text as legacy colors

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<style::values::specified::Color> {
+            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)));
                 }
             }
 

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;

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(&current_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!(
+        "<html><body>\
+         <{tag} id=attr {attributes}>text</{tag}>\
+         <{tag} id=css style='{css}'>text</{tag}>\
+         </body></html>"
+    ))
+}
+
+#[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(
+        "<html><body text='#f00'><p id=p>text</p>\
+         <p id=css style='color: #ff0000'>text</p></body></html>",
+    );
+    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"));
+}