From 6fdf40dce7f2658516ea18442c8936de5f796c5d Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Mon, 18 Aug 2025 05:19:09 -0700 Subject: [PATCH] layout: Always build `Tag` and `BaseFragmentInfo` with `ServoThreadSafeLayoutNode` (#38680) This cleanup makes the interface a bit simpler and prevents problems where the pseudo-element information is not passed by accident. Testing: This should not change behavior, so is covered by existing tests. --------- Signed-off-by: Martin Robinson Signed-off-by: Oriol Brufau Co-authored-by: Oriol Brufau --- components/layout/display_list/mod.rs | 13 +- components/layout/dom_traversal.rs | 63 +--------- .../layout/fragment_tree/base_fragment.rs | 117 ++++++++++++++---- components/layout/layout_impl.rs | 2 +- components/layout/replaced.rs | 31 +++-- components/layout/table/mod.rs | 3 +- components/script/layout_dom/element.rs | 4 +- 7 files changed, 117 insertions(+), 116 deletions(-) diff --git a/components/layout/display_list/mod.rs b/components/layout/display_list/mod.rs index ccdf243493f..7b1d4141630 100644 --- a/components/layout/display_list/mod.rs +++ b/components/layout/display_list/mod.rs @@ -12,7 +12,6 @@ use compositing_traits::display_list::{CompositorDisplayListInfo, SpatialTreeNod use euclid::{Point2D, Scale, SideOffsets2D, Size2D, UnknownUnit, Vector2D}; use fonts::GlyphStore; use gradient::WebRenderGradient; -use layout_api::ReflowRequest; use net_traits::image_cache::Image as CachedImage; use range::Range as ServoRange; use servo_arc::Arc as ServoArc; @@ -145,7 +144,11 @@ struct HighlightTraversalState { impl InspectorHighlight { fn for_node(node: OpaqueNode) -> Self { Self { - tag: Tag::new(node), + tag: Tag { + node, + // TODO: Support highlighting pseudo-elements. + pseudo_element_chain: Default::default(), + }, state: None, } } @@ -153,11 +156,11 @@ impl InspectorHighlight { impl DisplayListBuilder<'_> { pub(crate) fn build( - reflow_request: &ReflowRequest, stacking_context_tree: &mut StackingContextTree, fragment_tree: &FragmentTree, image_resolver: Arc, device_pixel_ratio: Scale, + highlighted_dom_node: Option, debug: &DebugOptions, ) -> BuiltDisplayList { // Build the rest of the display list which inclues all of the WebRender primitives. @@ -184,9 +187,7 @@ impl DisplayListBuilder<'_> { current_clip_id: ClipId::INVALID, webrender_display_list_builder: &mut webrender_display_list_builder, compositor_info, - inspector_highlight: reflow_request - .highlighted_dom_node - .map(InspectorHighlight::for_node), + inspector_highlight: highlighted_dom_node.map(InspectorHighlight::for_node), paint_body_background: true, clip_map: Default::default(), image_resolver, diff --git a/components/layout/dom_traversal.rs b/components/layout/dom_traversal.rs index e817e626ac5..4c48aeb97c3 100644 --- a/components/layout/dom_traversal.rs +++ b/components/layout/dom_traversal.rs @@ -5,7 +5,7 @@ use std::borrow::Cow; use fonts::ByteIndex; -use html5ever::{LocalName, local_name}; +use html5ever::LocalName; use layout_api::wrapper_traits::{ PseudoElementChain, ThreadSafeLayoutElement, ThreadSafeLayoutNode, }; @@ -23,7 +23,6 @@ use style::values::specified::Quotes; use crate::context::LayoutContext; use crate::dom::{BoxSlot, LayoutBox, NodeExt}; use crate::flow::inline::SharedInlineStyles; -use crate::fragment_tree::{BaseFragmentInfo, FragmentFlags, Tag}; use crate::quotes::quotes_for_lang; use crate::replaced::ReplacedContents; use crate::style_ext::{Display, DisplayGeneratingBox, DisplayInside, DisplayOutside}; @@ -73,66 +72,6 @@ impl<'dom> NodeAndStyleInfo<'dom> { } } -impl<'dom> From<&NodeAndStyleInfo<'dom>> for BaseFragmentInfo { - fn from(info: &NodeAndStyleInfo<'dom>) -> Self { - let threadsafe_node = info.node; - let pseudo_element_chain = info.node.pseudo_element_chain(); - let mut flags = FragmentFlags::empty(); - - // Anonymous boxes should not have a tag, because they should not take part in hit testing. - // - // TODO(mrobinson): It seems that anonymous boxes should take part in hit testing in some - // cases, but currently this means that the order of hit test results isn't as expected for - // some WPT tests. This needs more investigation. - if matches!( - pseudo_element_chain.innermost(), - Some(PseudoElement::ServoAnonymousBox) | - Some(PseudoElement::ServoAnonymousTable) | - Some(PseudoElement::ServoAnonymousTableCell) | - Some(PseudoElement::ServoAnonymousTableRow) - ) { - return Self::anonymous(); - } - - 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); - }, - _ => {}, - } - - if matches!( - element.type_id(), - Some(LayoutNodeType::Element( - LayoutElementType::HTMLInputElement | LayoutElementType::HTMLTextAreaElement - )) - ) { - flags.insert(FragmentFlags::IS_TEXT_CONTROL); - } - - if ThreadSafeLayoutElement::is_root(&element) { - flags.insert(FragmentFlags::IS_ROOT_ELEMENT); - } - }; - - Self { - tag: Some(Tag::new_pseudo( - threadsafe_node.opaque(), - pseudo_element_chain, - )), - flags, - } - } -} - #[derive(Debug)] pub(super) enum Contents { /// Any kind of content that is not replaced, including the contents of pseudo-elements. diff --git a/components/layout/fragment_tree/base_fragment.rs b/components/layout/fragment_tree/base_fragment.rs index 326a582b058..45d6ac59e92 100644 --- a/components/layout/fragment_tree/base_fragment.rs +++ b/components/layout/fragment_tree/base_fragment.rs @@ -3,11 +3,18 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use bitflags::bitflags; -use layout_api::combine_id_with_fragment_type; -use layout_api::wrapper_traits::PseudoElementChain; +use html5ever::local_name; +use layout_api::wrapper_traits::{ + PseudoElementChain, ThreadSafeLayoutElement, ThreadSafeLayoutNode, +}; +use layout_api::{LayoutElementType, LayoutNodeType, combine_id_with_fragment_type}; use malloc_size_of::malloc_size_of_is_0; use malloc_size_of_derive::MallocSizeOf; +use script::layout_dom::ServoThreadSafeLayoutNode; use style::dom::OpaqueNode; +use style::selector_parser::PseudoElement; + +use crate::dom_traversal::NodeAndStyleInfo; /// This data structure stores fields that are common to all non-base /// Fragment types and should generally be the first member of all @@ -48,19 +55,84 @@ pub(crate) struct BaseFragmentInfo { } impl BaseFragmentInfo { - pub(crate) fn new_for_node(node: OpaqueNode) -> Self { - Self { - tag: Some(Tag::new(node)), - flags: FragmentFlags::empty(), - } - } - pub(crate) fn anonymous() -> Self { Self { tag: None, flags: FragmentFlags::empty(), } } + + pub(crate) fn new_for_testing(id: usize) -> Self { + Self { + tag: Some(Tag { + node: OpaqueNode(id), + pseudo_element_chain: Default::default(), + }), + flags: FragmentFlags::empty(), + } + } +} + +impl From<&NodeAndStyleInfo<'_>> for BaseFragmentInfo { + fn from(info: &NodeAndStyleInfo) -> Self { + info.node.into() + } +} + +impl From> for BaseFragmentInfo { + fn from(node: ServoThreadSafeLayoutNode) -> Self { + let pseudo_element_chain = node.pseudo_element_chain(); + let mut flags = FragmentFlags::empty(); + + // Anonymous boxes should not have a tag, because they should not take part in hit testing. + // + // TODO(mrobinson): It seems that anonymous boxes should take part in hit testing in some + // cases, but currently this means that the order of hit test results isn't as expected for + // some WPT tests. This needs more investigation. + if matches!( + pseudo_element_chain.innermost(), + Some(PseudoElement::ServoAnonymousBox) | + Some(PseudoElement::ServoAnonymousTable) | + Some(PseudoElement::ServoAnonymousTableCell) | + Some(PseudoElement::ServoAnonymousTableRow) + ) { + return Self::anonymous(); + } + + if let Some(element) = 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); + }, + _ => {}, + } + + if matches!( + element.type_id(), + Some(LayoutNodeType::Element( + LayoutElementType::HTMLInputElement | LayoutElementType::HTMLTextAreaElement + )) + ) { + flags.insert(FragmentFlags::IS_TEXT_CONTROL); + } + + if ThreadSafeLayoutElement::is_root(&element) { + flags.insert(FragmentFlags::IS_ROOT_ELEMENT); + } + }; + + Self { + tag: Some(node.into()), + flags, + } + } } impl From for BaseFragment { @@ -118,25 +190,16 @@ pub(crate) struct Tag { } impl Tag { - /// Create a new Tag for a non-pseudo element. This is mainly used for - /// matching existing tags, since it does not accept an `info` argument. - pub(crate) fn new(node: OpaqueNode) -> Self { - Tag { - node, - pseudo_element_chain: Default::default(), - } - } - - /// Create a new Tag for a pseudo element. This is mainly used for - /// matching existing tags, since it does not accept an `info` argument. - pub(crate) fn new_pseudo(node: OpaqueNode, pseudo_element_chain: PseudoElementChain) -> Self { - Tag { - node, - pseudo_element_chain, - } - } - pub(crate) fn to_display_list_fragment_id(self) -> u64 { combine_id_with_fragment_type(self.node.id(), self.pseudo_element_chain.primary.into()) } } + +impl From> for Tag { + fn from(node: ServoThreadSafeLayoutNode<'_>) -> Self { + Self { + node: node.opaque(), + pseudo_element_chain: node.pseudo_element_chain(), + } + } +} diff --git a/components/layout/layout_impl.rs b/components/layout/layout_impl.rs index a6194ac61d5..697ed3cf89d 100644 --- a/components/layout/layout_impl.rs +++ b/components/layout/layout_impl.rs @@ -1200,11 +1200,11 @@ impl LayoutThread { stacking_context_tree.compositor_info.epoch = epoch.into(); let built_display_list = DisplayListBuilder::build( - reflow_request, stacking_context_tree, fragment_tree, image_resolver.clone(), self.device().device_pixel_ratio(), + reflow_request.highlighted_dom_node, &self.debug, ); self.compositor_api.send_display_list( diff --git a/components/layout/replaced.rs b/components/layout/replaced.rs index f66ad248c68..727829514b1 100644 --- a/components/layout/replaced.rs +++ b/components/layout/replaced.rs @@ -132,13 +132,13 @@ pub(crate) enum ReplacedContentKind { impl ReplacedContents { pub fn for_element( - element: ServoThreadSafeLayoutNode<'_>, + node: ServoThreadSafeLayoutNode<'_>, context: &LayoutContext, ) -> Option { - if let Some(ref data_attribute_string) = element.as_typeless_object_with_data_attribute() { + if let Some(ref data_attribute_string) = node.as_typeless_object_with_data_attribute() { if let Some(url) = try_to_parse_image_data_url(data_attribute_string) { return Self::from_image_url( - element, + node, context, &ComputedUrl::Valid(ServoArc::new(url)), ); @@ -146,17 +146,17 @@ impl ReplacedContents { } let (kind, natural_size) = { - if let Some((image, natural_size_in_dots)) = element.as_image() { + if let Some((image, natural_size_in_dots)) = node.as_image() { ( ReplacedContentKind::Image(image), NaturalSizes::from_natural_size_in_dots(natural_size_in_dots), ) - } else if let Some((canvas_info, natural_size_in_dots)) = element.as_canvas() { + } else if let Some((canvas_info, natural_size_in_dots)) = node.as_canvas() { ( ReplacedContentKind::Canvas(canvas_info), NaturalSizes::from_natural_size_in_dots(natural_size_in_dots), ) - } else if let Some((pipeline_id, browsing_context_id)) = element.as_iframe() { + } else if let Some((pipeline_id, browsing_context_id)) = node.as_iframe() { ( ReplacedContentKind::IFrame(IFrameInfo { pipeline_id, @@ -164,20 +164,20 @@ impl ReplacedContents { }), NaturalSizes::empty(), ) - } else if let Some((image_key, natural_size_in_dots)) = element.as_video() { + } else if let Some((image_key, natural_size_in_dots)) = node.as_video() { ( ReplacedContentKind::Video(image_key.map(|key| VideoInfo { image_key: key })), natural_size_in_dots .map_or_else(NaturalSizes::empty, NaturalSizes::from_natural_size_in_dots), ) - } else if let Some(svg_data) = element.as_svg() { + } else if let Some(svg_data) = node.as_svg() { let svg_source = match svg_data.source { None => { // The SVGSVGElement is not yet serialized, so we add it to a list // and hand it over to script to peform the serialization. context .image_resolver - .queue_svg_element_for_serialization(element); + .queue_svg_element_for_serialization(node); return None; }, Some(Err(_)) => { @@ -189,7 +189,7 @@ impl ReplacedContents { let result = context .image_resolver - .get_cached_image_for_url(element.opaque(), svg_source, UsePlaceholder::No) + .get_cached_image_for_url(node.opaque(), svg_source, UsePlaceholder::No) .ok(); let vector_image = result.map(|result| match result { @@ -210,25 +210,24 @@ impl ReplacedContents { if let ReplacedContentKind::Image(Some(Image::Raster(ref image))) = kind { context .image_resolver - .handle_animated_image(element.opaque(), image.clone()); + .handle_animated_image(node.opaque(), image.clone()); } - let base_fragment_info = BaseFragmentInfo::new_for_node(element.opaque()); Some(Self { kind, natural_size, - base_fragment_info, + base_fragment_info: node.into(), }) } pub fn from_image_url( - element: ServoThreadSafeLayoutNode<'_>, + node: ServoThreadSafeLayoutNode<'_>, context: &LayoutContext, image_url: &ComputedUrl, ) -> Option { if let ComputedUrl::Valid(image_url) = image_url { let (image, width, height) = match context.image_resolver.get_or_request_image_or_meta( - element.opaque(), + node.opaque(), image_url.clone().into(), UsePlaceholder::No, ) { @@ -251,7 +250,7 @@ impl ReplacedContents { return Some(Self { kind: ReplacedContentKind::Image(image), natural_size: NaturalSizes::from_width_and_height(width, height), - base_fragment_info: BaseFragmentInfo::new_for_node(element.opaque()), + base_fragment_info: node.into(), }); } None diff --git a/components/layout/table/mod.rs b/components/layout/table/mod.rs index 7f0b4d8cd9c..c44f23a31d8 100644 --- a/components/layout/table/mod.rs +++ b/components/layout/table/mod.rs @@ -82,7 +82,6 @@ use style::context::SharedStyleContext; use style::properties::ComputedValues; use style::properties::style_structs::Font; use style::selector_parser::PseudoElement; -use style_traits::dom::OpaqueNode; use super::flow::BlockFormattingContext; use crate::SharedStyle; @@ -232,7 +231,7 @@ impl TableSlotCell { pub fn mock_for_testing(id: usize, colspan: usize, rowspan: usize) -> Self { Self { base: LayoutBoxBase::new( - BaseFragmentInfo::new_for_node(OpaqueNode(id)), + BaseFragmentInfo::new_for_testing(id), ComputedValues::initial_values_with_font_override(Font::initial_values()).to_arc(), ), contents: BlockFormattingContext { diff --git a/components/script/layout_dom/element.rs b/components/script/layout_dom/element.rs index 1ab90d40aa0..e9c6bb16b4c 100644 --- a/components/script/layout_dom/element.rs +++ b/components/script/layout_dom/element.rs @@ -969,10 +969,10 @@ impl<'dom> ::selectors::Element for ServoLayoutElement<'dom> { /// ever access safe properties and cannot race on elements. #[derive(Clone, Copy, Debug)] pub struct ServoThreadSafeLayoutElement<'dom> { - /// The wrapped [`ServoLayoutNode`]. + /// The wrapped [`ServoLayoutElement`]. pub(super) element: ServoLayoutElement<'dom>, - /// The possibly nested [`PseudoElementChain`] for this node. + /// The possibly nested [`PseudoElementChain`] for this element. pub(super) pseudo_element_chain: PseudoElementChain, }