From f4c9b310d509155ec207f99426d9ba22dd8a06fd Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Mon, 10 Jun 2024 17:05:57 +0200 Subject: [PATCH] layout: Take into account `display: table` etc in offset* queries (#32448) * layout: Take into account `display: table` etc in offset* queries The specification says that for deciding whether an element should be used for offset* queries, a browser should take into account whether the element is a table cell or table. This change makes that happen. Co-authored-by: Oriol Brufau * Only tag HTML elements if they are in the HTML namespace --------- Co-authored-by: Oriol Brufau --- components/layout_2020/dom_traversal.rs | 23 ++++-- .../fragment_tree/base_fragment.rs | 10 ++- components/layout_2020/query.rs | 44 ++++++----- components/script/layout_dom/element.rs | 6 +- components/script/layout_dom/node.rs | 5 ++ .../shared/script_layout/wrapper_traits.rs | 3 + .../blocks/align-content-table-cell.html.ini | 9 --- .../css-tables/tentative/baseline-td.html.ini | 3 - .../tentative/element-sizing.html.ini | 3 - .../tentative/td-box-sizing-003.html.ini | 3 - ...-alignment-and-overflow.tentative.html.ini | 78 ------------------- 11 files changed, 60 insertions(+), 127 deletions(-) delete mode 100644 tests/wpt/meta/css/css-tables/tentative/element-sizing.html.ini diff --git a/components/layout_2020/dom_traversal.rs b/components/layout_2020/dom_traversal.rs index 9dc899ae4f0..95497c2b2e3 100644 --- a/components/layout_2020/dom_traversal.rs +++ b/components/layout_2020/dom_traversal.rs @@ -89,14 +89,21 @@ where }); let threadsafe_node = node.to_threadsafe(); - let flags = match threadsafe_node.as_element() { - Some(element) if element.is_body_element_of_html_element_root() => { - FragmentFlags::IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT - }, - Some(element) if element.get_local_name() == &local_name!("br") => { - FragmentFlags::IS_BR_ELEMENT - }, - _ => FragmentFlags::empty(), + let mut flags = FragmentFlags::empty(); + + if let Some(element) = threadsafe_node.as_html_element() { + if element.is_body_element_of_html_element_root() { + flags.insert(FragmentFlags::IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT); + } + match element.get_local_name() { + &local_name!("br") => { + flags.insert(FragmentFlags::IS_BR_ELEMENT); + }, + &local_name!("table") | &local_name!("th") | &local_name!("td") => { + flags.insert(FragmentFlags::IS_TABLE_TH_OR_TD_ELEMENT); + }, + _ => {}, + } }; Self { diff --git a/components/layout_2020/fragment_tree/base_fragment.rs b/components/layout_2020/fragment_tree/base_fragment.rs index d7081cd0403..dacc082581d 100644 --- a/components/layout_2020/fragment_tree/base_fragment.rs +++ b/components/layout_2020/fragment_tree/base_fragment.rs @@ -88,15 +88,19 @@ bitflags! { const IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT = 0b00000001; /// Whether or not the node that created this Fragment is a `
` element. const IS_BR_ELEMENT = 0b00000010; + /// Whether or not the node that created was a ``, `
` or + /// `` element. Note that this does *not* include elements with + /// `display: table` or `display: table-cell`. + const IS_TABLE_TH_OR_TD_ELEMENT = 0b00000100; /// Whether or not this Fragment was created to contain a replaced element or is /// a replaced element. - const IS_REPLACED = 0b00000100; + const IS_REPLACED = 0b00001000; /// Whether or not this Fragment was created to contain a list item marker /// with a used value of `list-style-position: outside`. - const IS_OUTSIDE_LIST_ITEM_MARKER = 0b00001000; + const IS_OUTSIDE_LIST_ITEM_MARKER = 0b00010000; /// Avoid painting the fragment, this is used for empty table cells when 'empty-cells' is 'hide'. /// This flag doesn't avoid hit-testing. - const DO_NOT_PAINT = 0b00010000; + const DO_NOT_PAINT = 0b00100000; } } diff --git a/components/layout_2020/query.rs b/components/layout_2020/query.rs index d950696408c..6de17b7dd2a 100644 --- a/components/layout_2020/query.rs +++ b/components/layout_2020/query.rs @@ -31,7 +31,7 @@ use style::traversal::resolve_style; use style::values::generics::font::LineHeight; use style_traits::{ParsingMode, ToCss}; -use crate::fragment_tree::{Fragment, FragmentFlags, FragmentTree, Tag}; +use crate::fragment_tree::{BoxFragment, Fragment, FragmentFlags, FragmentTree, Tag}; pub fn process_content_box_request( requested_node: OpaqueNode, @@ -428,24 +428,7 @@ fn process_offset_parent_query_inner( // Record the paths of the nodes being traversed. let parent_node_address = match fragment { Fragment::Box(fragment) | Fragment::Float(fragment) => { - let is_eligible_parent = - match (is_body_element, fragment.style.get_box().position) { - // Spec says the element is eligible as `offsetParent` if any of - // these are true: - // 1) Is the body element - // 2) Is static position *and* is a table or table cell - // 3) Is not static position - // TODO: Handle case 2 - (true, _) | - (false, Position::Absolute) | - (false, Position::Fixed) | - (false, Position::Relative) | - (false, Position::Sticky) => true, - - // Otherwise, it's not a valid parent - (false, Position::Static) => false, - }; - + let is_eligible_parent = is_eligible_parent(fragment); match base.tag { Some(tag) if is_eligible_parent && !tag.is_pseudo() => Some(tag.node), _ => None, @@ -531,6 +514,29 @@ fn process_offset_parent_query_inner( }) } +/// Returns whether or not the element with the given style and body element determination +/// is eligible to be a parent element for offset* queries. +/// +/// From : +/// > +/// > Return the nearest ancestor element of the element for which at least one of the following is +/// > true and terminate this algorithm if such an ancestor is found: +/// > 1. The computed value of the position property is not static. +/// > 2. It is the HTML body element. +/// > 3. The computed value of the position property of the element is static and the ancestor is +/// > one of the following HTML elements: td, th, or table. +fn is_eligible_parent(fragment: &BoxFragment) -> bool { + fragment + .base + .flags + .contains(FragmentFlags::IS_BODY_ELEMENT_OF_HTML_ELEMENT_ROOT) || + fragment.style.get_box().position != Position::Static || + fragment + .base + .flags + .contains(FragmentFlags::IS_TABLE_TH_OR_TD_ELEMENT) +} + // https://html.spec.whatwg.org/multipage/#the-innertext-idl-attribute pub fn process_element_inner_text_query<'dom>(_node: impl LayoutNode<'dom>) -> String { "".to_owned() diff --git a/components/script/layout_dom/element.rs b/components/script/layout_dom/element.rs index 3e71e1eb773..55855930a31 100644 --- a/components/script/layout_dom/element.rs +++ b/components/script/layout_dom/element.rs @@ -69,6 +69,10 @@ impl<'dom> ServoLayoutElement<'dom> { ServoLayoutElement { element: el } } + pub(super) fn is_html_element(&self) -> bool { + self.element.is_html_element() + } + #[inline] fn get_attr_enum(&self, namespace: &Namespace, name: &LocalName) -> Option<&AttrValue> { self.element.get_attr_for_layout(namespace, name) @@ -139,7 +143,7 @@ impl<'dom> style::dom::TElement for ServoLayoutElement<'dom> { } fn is_html_element(&self) -> bool { - self.element.is_html_element() + ServoLayoutElement::is_html_element(self) } fn is_mathml_element(&self) -> bool { diff --git a/components/script/layout_dom/node.rs b/components/script/layout_dom/node.rs index fdbe615513c..f4e014a6a1b 100644 --- a/components/script/layout_dom/node.rs +++ b/components/script/layout_dom/node.rs @@ -313,6 +313,11 @@ impl<'dom> ThreadSafeLayoutNode<'dom> for ServoThreadSafeLayoutNode<'dom> { }) } + fn as_html_element(&self) -> Option> { + self.as_element() + .filter(|element| element.element.is_html_element()) + } + fn style_data(&self) -> Option<&'dom StyleData> { self.node.style_data() } diff --git a/components/shared/script_layout/wrapper_traits.rs b/components/shared/script_layout/wrapper_traits.rs index 399c8bd3362..fe3fc69f47e 100644 --- a/components/shared/script_layout/wrapper_traits.rs +++ b/components/shared/script_layout/wrapper_traits.rs @@ -230,6 +230,9 @@ pub trait ThreadSafeLayoutNode<'dom>: Clone + Copy + Debug + NodeInfo + PartialE /// Returns a ThreadSafeLayoutElement if this is an element, None otherwise. fn as_element(&self) -> Option; + /// Returns a ThreadSafeLayoutElement if this is an element in an HTML namespace, None otherwise. + fn as_html_element(&self) -> Option; + #[inline] fn get_pseudo_element_type(&self) -> PseudoElementType { self.as_element() diff --git a/tests/wpt/meta/css/css-align/blocks/align-content-table-cell.html.ini b/tests/wpt/meta/css/css-align/blocks/align-content-table-cell.html.ini index 3c507a0a4f1..f4cd680b445 100644 --- a/tests/wpt/meta/css/css-align/blocks/align-content-table-cell.html.ini +++ b/tests/wpt/meta/css/css-align/blocks/align-content-table-cell.html.ini @@ -2,20 +2,11 @@ [vertical-align:top and align-content:start are equivalent] expected: FAIL - [vertical-align:middle and `align-content:unsafe center` are equivalent] - expected: FAIL - [vertical-align:bottom and `align-content:unsafe end` are equivalent] expected: FAIL [vertical-align:baseline and align-content:baseline are equivalent] expected: FAIL - [vertical-align:middle and `align-content:safe center` are equivalent if the container is tall] - expected: FAIL - [vertical-align:bottom and `align-content:safe end` are equivalent if the container is tall] expected: FAIL - - [`align-content:safe center` is equivalent to `unsafe center` even if the specified `height` is short] - expected: FAIL diff --git a/tests/wpt/meta/css/css-tables/tentative/baseline-td.html.ini b/tests/wpt/meta/css/css-tables/tentative/baseline-td.html.ini index 194bf59ed5b..a562bb9e0aa 100644 --- a/tests/wpt/meta/css/css-tables/tentative/baseline-td.html.ini +++ b/tests/wpt/meta/css/css-tables/tentative/baseline-td.html.ini @@ -1,6 +1,3 @@ [baseline-td.html] - [table, .display-table 1] - expected: FAIL - [table, .display-table 3] expected: FAIL diff --git a/tests/wpt/meta/css/css-tables/tentative/element-sizing.html.ini b/tests/wpt/meta/css/css-tables/tentative/element-sizing.html.ini deleted file mode 100644 index 03e6e778159..00000000000 --- a/tests/wpt/meta/css/css-tables/tentative/element-sizing.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[element-sizing.html] - [table 2] - expected: FAIL diff --git a/tests/wpt/meta/css/css-tables/tentative/td-box-sizing-003.html.ini b/tests/wpt/meta/css/css-tables/tentative/td-box-sizing-003.html.ini index 06e1f46f468..683e8b69ce3 100644 --- a/tests/wpt/meta/css/css-tables/tentative/td-box-sizing-003.html.ini +++ b/tests/wpt/meta/css/css-tables/tentative/td-box-sizing-003.html.ini @@ -1,7 +1,4 @@ [td-box-sizing-003.html] - [table 8] - expected: FAIL - [table 9] expected: FAIL diff --git a/tests/wpt/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini b/tests/wpt/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini index 7f1dd8e36a9..f13df0d7bbf 100644 --- a/tests/wpt/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini +++ b/tests/wpt/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini @@ -215,42 +215,6 @@ [] expected: FAIL - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - [] expected: FAIL @@ -263,12 +227,6 @@ [] expected: FAIL - [] - expected: FAIL - - [] - expected: FAIL - [] expected: FAIL @@ -281,12 +239,6 @@ [] expected: FAIL - [] - expected: FAIL - - [] - expected: FAIL - [] expected: FAIL @@ -326,39 +278,9 @@ [] expected: FAIL - [] - expected: FAIL - - [] - expected: FAIL - [] expected: FAIL - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - - [] - expected: FAIL - [] expected: FAIL