From 0d21992edd8783e2e9b5c224fdd60a4e8e3a193e Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Thu, 1 May 2025 15:25:34 +0200 Subject: [PATCH] script: Clamp table spans according to the HTML specification (#36703) Previously, spans were partially clamped during layout, but this means that accessing and setting these properties via script wouldn't behave according to the HTML specification. In addition, the value wasn't floored in layout, so could lead to panics. This change improves clamping and moves it to script. Testing: This change includes a new WPT test. Fixes #36699. Signed-off-by: Martin Robinson --- components/layout/table/construct.rs | 27 +++++---- components/script/dom/htmltablecellelement.rs | 35 +++++++----- components/script/dom/htmltablecolelement.rs | 17 +++--- components/script/dom/macros.rs | 20 +++++++ tests/wpt/meta/MANIFEST.json | 11 +++- .../meta/html/dom/reflection-tabular.html.ini | 36 ------------ .../css/css-tables/colspan-zero-crash.html | 10 ++++ .../processing-model-1/col-span-limits.html | 55 ++++++++++++++++++- .../processing-model-1/span-limits.html | 41 ++++++++++++++ 9 files changed, 177 insertions(+), 75 deletions(-) create mode 100644 tests/wpt/tests/css/css-tables/colspan-zero-crash.html diff --git a/components/layout/table/construct.rs b/components/layout/table/construct.rs index f20360d3b56..56e11320be4 100644 --- a/components/layout/table/construct.rs +++ b/components/layout/table/construct.rs @@ -1019,15 +1019,16 @@ where DisplayLayoutInternal::TableCell => { // This value will already have filtered out rowspan=0 // in quirks mode, so we don't have to worry about that. - // - // The HTML specification limits the parsed value of `rowspan` to - // 65534 and `colspan` to 1000, so we also enforce the same limits - // when dealing with arbitrary DOM elements (perhaps created via - // script). let (rowspan, colspan) = if info.pseudo_element_type.is_none() { let node = info.node.to_threadsafe(); - let rowspan = node.get_rowspan().unwrap_or(1).min(65534) as usize; - let colspan = node.get_colspan().unwrap_or(1).min(1000) as usize; + let rowspan = node.get_rowspan().unwrap_or(1) as usize; + let colspan = node.get_colspan().unwrap_or(1) as usize; + + // The HTML specification clamps value of `rowspan` to [0, 65534] and + // `colspan` to [1, 1000]. + assert!((1..=1000).contains(&colspan)); + assert!((0..=65534).contains(&rowspan)); + (rowspan, colspan) } else { (1, 1) @@ -1140,21 +1141,19 @@ fn add_column<'dom, Node: NodeExt<'dom>>( is_anonymous: bool, ) -> ArcRefCell { let span = if column_info.pseudo_element_type.is_none() { - column_info - .node - .to_threadsafe() - .get_span() - .unwrap_or(1) - .min(1000) as usize + column_info.node.to_threadsafe().get_span().unwrap_or(1) } else { 1 }; + // The HTML specification clamps value of `span` for `` to [1, 1000]. + assert!((1..=1000).contains(&span)); + let column = ArcRefCell::new(TableTrack { base: LayoutBoxBase::new(column_info.into(), column_info.style.clone()), group_index, is_anonymous, }); - collection.extend(repeat(column.clone()).take(span)); + collection.extend(repeat(column.clone()).take(span as usize)); column } diff --git a/components/script/dom/htmltablecellelement.rs b/components/script/dom/htmltablecellelement.rs index 4f312e928c4..8b553923230 100644 --- a/components/script/dom/htmltablecellelement.rs +++ b/components/script/dom/htmltablecellelement.rs @@ -70,13 +70,17 @@ impl HTMLTableCellElementMethods for HTMLTableCellElement make_uint_getter!(ColSpan, "colspan", DEFAULT_COLSPAN); // https://html.spec.whatwg.org/multipage/#dom-tdth-colspan - make_uint_setter!(SetColSpan, "colspan", DEFAULT_COLSPAN); + // > The colSpan IDL attribute must reflect the colspan content attribute. It is clamped to + // > the range [1, 1000], and its default value is 1. + make_clamped_uint_setter!(SetColSpan, "colspan", 1, 1000, 1); // https://html.spec.whatwg.org/multipage/#dom-tdth-rowspan make_uint_getter!(RowSpan, "rowspan", DEFAULT_ROWSPAN); // https://html.spec.whatwg.org/multipage/#dom-tdth-rowspan - make_uint_setter!(SetRowSpan, "rowspan", DEFAULT_ROWSPAN); + // > The rowSpan IDL attribute must reflect the rowspan content attribute. It is clamped to + // > the range [0, 65534], and its default value is 1. + make_clamped_uint_setter!(SetRowSpan, "rowspan", 0, 65534, 1); // https://html.spec.whatwg.org/multipage/#dom-tdth-bgcolor make_getter!(BgColor, "bgcolor"); @@ -174,23 +178,26 @@ impl VirtualMethods for HTMLTableCellElement { match *local_name { local_name!("colspan") => { let mut attr = AttrValue::from_u32(value.into(), DEFAULT_COLSPAN); - if let AttrValue::UInt(_, ref mut val) = attr { - if *val == 0 { - *val = 1; - } + if let AttrValue::UInt(_, ref mut value) = attr { + // From : + // > The colSpan IDL attribute must reflect the colspan content attribute. It is clamped to + // > the range [1, 1000], and its default value is 1. + *value = (*value).clamp(1, 1000); } attr }, local_name!("rowspan") => { let mut attr = AttrValue::from_u32(value.into(), DEFAULT_ROWSPAN); - if let AttrValue::UInt(_, ref mut val) = attr { - if *val == 0 { - let node = self.upcast::(); - let doc = node.owner_doc(); - // rowspan = 0 is not supported in quirks mode - if doc.quirks_mode() != QuirksMode::NoQuirks { - *val = 1; - } + if let AttrValue::UInt(_, ref mut value) = attr { + // From : + // > The rowSpan IDL attribute must reflect the rowspan content attribute. It is clamped to + // > the range [0, 65534], and its default value is 1. + // Note that rowspan = 0 is not supported in quirks mode. + let document = self.upcast::().owner_doc(); + if document.quirks_mode() != QuirksMode::NoQuirks { + *value = (*value).clamp(1, 65534); + } else { + *value = (*value).clamp(0, 65534); } } attr diff --git a/components/script/dom/htmltablecolelement.rs b/components/script/dom/htmltablecolelement.rs index c7ad4afd944..9e8eecf1147 100644 --- a/components/script/dom/htmltablecolelement.rs +++ b/components/script/dom/htmltablecolelement.rs @@ -20,8 +20,6 @@ use crate::dom::node::Node; use crate::dom::virtualmethods::VirtualMethods; use crate::script_runtime::CanGc; -const DEFAULT_SPAN: u32 = 1; - #[dom_struct] pub(crate) struct HTMLTableColElement { htmlelement: HTMLElement, @@ -62,9 +60,11 @@ impl HTMLTableColElement { impl HTMLTableColElementMethods for HTMLTableColElement { // - make_uint_getter!(Span, "span", DEFAULT_SPAN); + make_uint_getter!(Span, "span", 1); // - make_uint_setter!(SetSpan, "span", DEFAULT_SPAN); + // > The span IDL attribute must reflect the content attribute of the same name. It is clamped + // > to the range [1, 1000], and its default value is 1. + make_clamped_uint_setter!(SetSpan, "span", 1, 1000, 1); } pub(crate) trait HTMLTableColElementLayoutHelpers<'dom> { @@ -96,11 +96,12 @@ impl VirtualMethods for HTMLTableColElement { fn parse_plain_attribute(&self, local_name: &LocalName, value: DOMString) -> AttrValue { match *local_name { local_name!("span") => { - let mut attr = AttrValue::from_u32(value.into(), DEFAULT_SPAN); + let mut attr = AttrValue::from_u32(value.into(), 1); if let AttrValue::UInt(_, ref mut val) = attr { - if *val == 0 { - *val = 1; - } + // From : + // > The span IDL attribute must reflect the content attribute of the same name. + // > It is clamped to the range [1, 1000], and its default value is 1. + *val = (*val).clamp(1, 1000); } attr }, diff --git a/components/script/dom/macros.rs b/components/script/dom/macros.rs index c2f5ba37c21..997341984c6 100644 --- a/components/script/dom/macros.rs +++ b/components/script/dom/macros.rs @@ -317,6 +317,26 @@ macro_rules! make_uint_setter( }; ); +#[macro_export] +macro_rules! make_clamped_uint_setter( + ($attr:ident, $htmlname:tt, $min:expr, $max:expr, $default:expr) => ( + fn $attr(&self, value: u32) { + use $crate::dom::bindings::inheritance::Castable; + use $crate::dom::element::Element; + use $crate::dom::values::UNSIGNED_LONG_MAX; + use $crate::script_runtime::CanGc; + let value = if value > UNSIGNED_LONG_MAX { + $default + } else { + value.clamp($min, $max) + }; + + let element = self.upcast::(); + element.set_uint_attribute(&html5ever::local_name!($htmlname), value, CanGc::note()) + } + ); +); + #[macro_export] macro_rules! make_limited_uint_setter( ($attr:ident, $htmlname:tt, $default:expr) => ( diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index 889b8353308..5385c0403b4 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -4916,6 +4916,13 @@ {} ] ], + "colspan-zero-crash.html": [ + "a50586a5bfa7551724b838a0438339a34b4930fb", + [ + null, + {} + ] + ], "crashtests": { "caption-repaint-crash.html": [ "6a024d0c1d7ef58da06a489d80d187bcb2a3e350", @@ -739709,14 +739716,14 @@ ], "processing-model-1": { "col-span-limits.html": [ - "a4a425b9c1f70926c77ad3eb1b8a8a87a4655de9", + "2a1ac80e65ad37a0e15ca383e67889b79f2308d0", [ null, {} ] ], "span-limits.html": [ - "cdfa61bbcdc06ea62b80d042440d55fb0c89a186", + "798639b387562a54965d5283673a50773c2a3c49", [ null, { diff --git a/tests/wpt/meta/html/dom/reflection-tabular.html.ini b/tests/wpt/meta/html/dom/reflection-tabular.html.ini index 7550e2dc15d..1aeec1b3ef7 100644 --- a/tests/wpt/meta/html/dom/reflection-tabular.html.ini +++ b/tests/wpt/meta/html/dom/reflection-tabular.html.ini @@ -1487,9 +1487,6 @@ [colgroup.tabIndex: IDL set to -2147483648] expected: FAIL - [colgroup.span: setAttribute() to 2147483647] - expected: FAIL - [colgroup.span: setAttribute() to 2147483648] expected: FAIL @@ -1499,9 +1496,6 @@ [colgroup.span: setAttribute() to 4294967296] expected: FAIL - [colgroup.span: setAttribute() to 1001] - expected: FAIL - [colgroup.span: IDL set to 0] expected: FAIL @@ -2276,9 +2270,6 @@ [col.tabIndex: IDL set to -2147483648] expected: FAIL - [col.span: setAttribute() to 2147483647] - expected: FAIL - [col.span: setAttribute() to 2147483648] expected: FAIL @@ -2288,9 +2279,6 @@ [col.span: setAttribute() to 4294967296] expected: FAIL - [col.span: setAttribute() to 1001] - expected: FAIL - [col.span: IDL set to 0] expected: FAIL @@ -5657,9 +5645,6 @@ [td.tabIndex: IDL set to -2147483648] expected: FAIL - [td.colSpan: setAttribute() to 2147483647] - expected: FAIL - [td.colSpan: setAttribute() to 2147483648] expected: FAIL @@ -5669,9 +5654,6 @@ [td.colSpan: setAttribute() to 4294967296] expected: FAIL - [td.colSpan: setAttribute() to 1001] - expected: FAIL - [td.colSpan: IDL set to 0] expected: FAIL @@ -5684,9 +5666,6 @@ [td.colSpan: IDL set to 1001] expected: FAIL - [td.rowSpan: setAttribute() to 2147483647] - expected: FAIL - [td.rowSpan: setAttribute() to 2147483648] expected: FAIL @@ -5696,9 +5675,6 @@ [td.rowSpan: setAttribute() to 4294967296] expected: FAIL - [td.rowSpan: setAttribute() to 65535] - expected: FAIL - [td.rowSpan: IDL set to 2147483647] expected: FAIL @@ -7157,9 +7133,6 @@ [th.tabIndex: IDL set to -2147483648] expected: FAIL - [th.colSpan: setAttribute() to 2147483647] - expected: FAIL - [th.colSpan: setAttribute() to 2147483648] expected: FAIL @@ -7169,9 +7142,6 @@ [th.colSpan: setAttribute() to 4294967296] expected: FAIL - [th.colSpan: setAttribute() to 1001] - expected: FAIL - [th.colSpan: IDL set to 0] expected: FAIL @@ -7184,9 +7154,6 @@ [th.colSpan: IDL set to 1001] expected: FAIL - [th.rowSpan: setAttribute() to 2147483647] - expected: FAIL - [th.rowSpan: setAttribute() to 2147483648] expected: FAIL @@ -7196,9 +7163,6 @@ [th.rowSpan: setAttribute() to 4294967296] expected: FAIL - [th.rowSpan: setAttribute() to 65535] - expected: FAIL - [th.rowSpan: IDL set to 2147483647] expected: FAIL diff --git a/tests/wpt/tests/css/css-tables/colspan-zero-crash.html b/tests/wpt/tests/css/css-tables/colspan-zero-crash.html new file mode 100644 index 00000000000..a50586a5bfa --- /dev/null +++ b/tests/wpt/tests/css/css-tables/colspan-zero-crash.html @@ -0,0 +1,10 @@ + + + + + + diff --git a/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html b/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html index a4a425b9c1f..2a1ac80e65a 100644 --- a/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html +++ b/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/col-span-limits.html @@ -39,12 +39,21 @@ These two must look the same, each having 2 cells in one row:
- +
+ + + + + + + + +
diff --git a/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/span-limits.html b/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/span-limits.html index cdfa61bbcdc..798639b3875 100644 --- a/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/span-limits.html +++ b/tests/wpt/tests/html/semantics/tabular-data/processing-model-1/span-limits.html @@ -29,6 +29,17 @@ + + + + + + + + + +
+