layout: Do not inherit node and fragment flags in anonymous boxes (#31586)

This doesn't really have observable behavior right now, as much as I tried to trigger some kind of bug. On the other hand, it's just wrong and is very obvious when you dump the Fragment tree. If you create a `display: table-cell` that is a child of the `<body>` all parts of the anonymous table are flagged as if they are the `<body>` element.
author: Martin Robinson <mrobinson@igalia.com> 2024-03-09 10:13:19 +0100
committer: GitHub <noreply@github.com> 2024-03-09 09:13:19 +0000
commit: 1f23ec2b27789c356a6283d9005079b6e9b1e66b (patch)
tree: df5256c59df043d12dd2630e2a3cf90e0a62672c
parent: 55f908653f6fb02c344459319a7ca87487cfa4bf (diff)
download: servo-1f23ec2b27789c356a6283d9005079b6e9b1e66b.tar.gz
servo-1f23ec2b27789c356a6283d9005079b6e9b1e66b.zip
7 files changed, 73 insertions, 38 deletions
diff --git a/components/layout_2020/dom_traversal.rs b/components/layout_2020/dom_traversal.rs
index c1226d7d1d8..b00108a2b9a 100644
--- a/components/layout_2020/dom_traversal.rs
+++ b/components/layout_2020/dom_traversal.rs
@@ -5,6 +5,7 @@
 use std::borrow::Cow;
 
 use html5ever::{local_name, LocalName};
+use log::warn;
 use script_layout_interface::wrapper_traits::{ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use servo_arc::Arc as ServoArc;
 use style::properties::ComputedValues;
@@ -27,7 +28,7 @@ pub(crate) enum WhichPseudoElement {
 /// avoid having to repeat the same arguments in argument lists.
 #[derive(Clone)]
 pub(crate) struct NodeAndStyleInfo<Node> {
-    pub node: Node,
+    pub node: Option<Node>,
     pub pseudo_element_type: Option<WhichPseudoElement>,
     pub style: ServoArc<ComputedValues>,
 }
@@ -39,7 +40,7 @@ impl<Node> NodeAndStyleInfo<Node> {
         style: ServoArc<ComputedValues>,
     ) -> Self {
         Self {
-            node,
+            node: Some(node),
             pseudo_element_type: Some(pseudo_element_type),
             style,
         }
@@ -47,7 +48,7 @@ impl<Node> NodeAndStyleInfo<Node> {
 
     pub(crate) fn new(node: Node, style: ServoArc<ComputedValues>) -> Self {
         Self {
-            node,
+            node: Some(node),
             pseudo_element_type: None,
             style,
         }
@@ -55,6 +56,14 @@ impl<Node> NodeAndStyleInfo<Node> {
 }
 
 impl<Node: Clone> NodeAndStyleInfo<Node> {
+    pub(crate) fn new_anonymous(&self, style: ServoArc<ComputedValues>) -> Self {
+        Self {
+            node: None,
+            pseudo_element_type: self.pseudo_element_type,
+            style,
+        }
+    }
+
     pub(crate) fn new_replacing_style(&self, style: ServoArc<ComputedValues>) -> Self {
         Self {
             node: self.node.clone(),
@@ -69,12 +78,17 @@ where
     Node: NodeExt<'dom>,
 {
     fn from(info: &NodeAndStyleInfo<Node>) -> Self {
+        let node = match info.node {
+            Some(node) => node,
+            None => return Self::anonymous(),
+        };
+
         let pseudo = info.pseudo_element_type.map(|pseudo| match pseudo {
             WhichPseudoElement::Before => PseudoElement::Before,
             WhichPseudoElement::After => PseudoElement::After,
         });
 
-        let threadsafe_node = info.node.to_threadsafe();
+        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
@@ -86,7 +100,7 @@ where
         };
 
         Self {
-            tag: Tag::new_pseudo(threadsafe_node.opaque(), pseudo),
+            tag: Some(Tag::new_pseudo(threadsafe_node.opaque(), pseudo)),
             flags,
         }
     }
@@ -302,8 +316,15 @@ impl NonReplacedContents {
     ) where
         Node: NodeExt<'dom>,
     {
+        let node = match info.node {
+            Some(node) => node,
+            None => {
+                warn!("Tried to traverse an anonymous node!");
+                return;
+            },
+        };
         match self {
-            NonReplacedContents::OfElement => traverse_children_of(info.node, context, handler),
+            NonReplacedContents::OfElement => traverse_children_of(node, context, handler),
             NonReplacedContents::OfPseudoElement(items) => {
                 traverse_pseudo_element_contents(info, context, handler, items)
             },
diff --git a/components/layout_2020/flexbox/construct.rs b/components/layout_2020/flexbox/construct.rs
index b5aa9b34c47..065e03989d3 100644
--- a/components/layout_2020/flexbox/construct.rs
+++ b/components/layout_2020/flexbox/construct.rs
@@ -160,9 +160,7 @@ where
                         self.context,
                         self.text_decoration_line,
                     );
-                    let info = &self
-                        .info
-                        .new_replacing_style(anonymous_style.clone().unwrap());
+                    let info = &self.info.new_anonymous(anonymous_style.clone().unwrap());
                     IndependentFormattingContext::NonReplaced(NonReplacedFormattingContext {
                         base_fragment_info: info.into(),
                         style: info.style.clone(),
diff --git a/components/layout_2020/flow/construct.rs b/components/layout_2020/flow/construct.rs
index 1a512910374..cd2dfffc4d0 100644
--- a/components/layout_2020/flow/construct.rs
+++ b/components/layout_2020/flow/construct.rs
@@ -317,7 +317,7 @@ where
             self.current_inline_level_boxes()
                 .push(ArcRefCell::new(InlineLevelBox::Atomic(ifc)));
         } else {
-            let anonymous_info = self.info.new_replacing_style(ifc.style().clone());
+            let anonymous_info = self.info.new_anonymous(ifc.style().clone());
             let table_block = ArcRefCell::new(BlockLevelBox::Independent(ifc));
             self.block_level_boxes.push(BlockLevelJob {
                 info: anonymous_info,
@@ -690,7 +690,7 @@ where
         );
         std::mem::swap(&mut self.ongoing_inline_formatting_context, &mut ifc);
 
-        let info = self.info.new_replacing_style(anonymous_style.clone());
+        let info = self.info.new_anonymous(anonymous_style.clone());
         self.block_level_boxes.push(BlockLevelJob {
             info,
             // FIXME(nox): We should be storing this somewhere.
diff --git a/components/layout_2020/fragment_tree/base_fragment.rs b/components/layout_2020/fragment_tree/base_fragment.rs
index a43e375f26c..d84dde30111 100644
--- a/components/layout_2020/fragment_tree/base_fragment.rs
+++ b/components/layout_2020/fragment_tree/base_fragment.rs
@@ -47,8 +47,8 @@ impl BaseFragment {
 /// Information necessary to construct a new BaseFragment.
 #[derive(Clone, Copy, Debug, Serialize)]
 pub(crate) struct BaseFragmentInfo {
-    /// The tag to use for the new BaseFragment.
-    pub tag: Tag,
+    /// The tag to use for the new BaseFragment, if it is not an anonymous Fragment.
+    pub tag: Option<Tag>,
 
     /// The flags to use for the new BaseFragment.
     pub flags: FragmentFlags,
@@ -57,7 +57,14 @@ pub(crate) struct BaseFragmentInfo {
 impl BaseFragmentInfo {
     pub(crate) fn new_for_node(node: OpaqueNode) -> Self {
         Self {
-            tag: Tag::new(node),
+            tag: Some(Tag::new(node)),
+            flags: FragmentFlags::empty(),
+        }
+    }
+
+    pub(crate) fn anonymous() -> Self {
+        Self {
+            tag: None,
             flags: FragmentFlags::empty(),
         }
     }
@@ -66,7 +73,7 @@ impl BaseFragmentInfo {
 impl From<BaseFragmentInfo> for BaseFragment {
     fn from(info: BaseFragmentInfo) -> Self {
         Self {
-            tag: Some(info.tag),
+            tag: info.tag,
             debug_id: DebugId::new(),
             flags: info.flags,
         }
diff --git a/components/layout_2020/lists.rs b/components/layout_2020/lists.rs
index f3ffe471a3e..1b337ac3555 100644
--- a/components/layout_2020/lists.rs
+++ b/components/layout_2020/lists.rs
@@ -2,6 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+use log::warn;
 use style::properties::longhands::list_style_type::computed_value::T as ListStyleType;
 use style::properties::style_structs;
 use style::values::computed::Image;
@@ -20,12 +21,19 @@ where
     Node: NodeExt<'dom>,
 {
     let style = info.style.get_list();
+    let node = match info.node {
+        Some(node) => node,
+        None => {
+            warn!("Tried to make a marker for an anonymous node!");
+            return None;
+        },
+    };
 
     // https://drafts.csswg.org/css-lists/#marker-image
     let marker_image = || match &style.list_style_image {
         Image::Url(url) => Some(vec![
             PseudoElementContentItem::Replaced(ReplacedContent::from_image_url(
-                info.node, context, url,
+                node, context, url,
             )?),
             PseudoElementContentItem::Text(" ".into()),
         ]),
diff --git a/components/layout_2020/table/construct.rs b/components/layout_2020/table/construct.rs
index fe419c04c71..b43861ff3c2 100644
--- a/components/layout_2020/table/construct.rs
+++ b/components/layout_2020/table/construct.rs
@@ -26,7 +26,7 @@ use crate::formatting_contexts::{
     IndependentFormattingContext, NonReplacedFormattingContext,
     NonReplacedFormattingContextContents,
 };
-use crate::fragment_tree::{BaseFragmentInfo, FragmentFlags, Tag};
+use crate::fragment_tree::BaseFragmentInfo;
 use crate::style_ext::{DisplayGeneratingBox, DisplayLayoutInternal};
 
 /// A reference to a slot and its coordinates in the table
@@ -87,7 +87,7 @@ impl Table {
                 &PseudoElement::ServoAnonymousTable,
                 &parent_info.style,
             );
-        let anonymous_info = parent_info.new_replacing_style(anonymous_style.clone());
+        let anonymous_info = parent_info.new_anonymous(anonymous_style.clone());
 
         let mut table_builder =
             TableBuilderTraversal::new(context, &anonymous_info, propagated_text_decoration_line);
@@ -642,7 +642,7 @@ where
                 &PseudoElement::ServoAnonymousTableCell,
                 &self.info.style,
             );
-        let anonymous_info = self.info.new_replacing_style(anonymous_style);
+        let anonymous_info = self.info.new_anonymous(anonymous_style);
         let mut row_builder =
             TableRowBuilder::new(self, &anonymous_info, self.current_text_decoration_line);
 
@@ -756,8 +756,12 @@ where
                     ::std::mem::forget(box_slot)
                 },
                 DisplayLayoutInternal::TableColumn => {
-                    let node = info.node.to_threadsafe();
-                    let span = (node.get_span().unwrap_or(1) as usize).min(1000);
+                    let span = info
+                        .node
+                        .and_then(|node| node.to_threadsafe().get_span())
+                        .unwrap_or(1)
+                        .min(1000);
+
                     for _ in 0..span + 1 {
                         self.builder.table.columns.push(TableTrack {
                             base_fragment_info: info.into(),
@@ -785,8 +789,11 @@ where
 
                     let first_column = self.builder.table.columns.len();
                     if column_group_builder.columns.is_empty() {
-                        let node = info.node.to_threadsafe();
-                        let span = (node.get_span().unwrap_or(1) as usize).min(1000);
+                        let span = info
+                            .node
+                            .and_then(|node| node.to_threadsafe().get_span())
+                            .unwrap_or(1)
+                            .min(1000) as usize;
 
                         self.builder.table.columns.extend(
                             repeat(TableTrack {
@@ -894,7 +901,7 @@ where
                 &PseudoElement::ServoAnonymousTableCell,
                 &self.info.style,
             );
-        let anonymous_info = self.info.new_replacing_style(anonymous_style);
+        let anonymous_info = self.info.new_anonymous(anonymous_style);
         let mut builder =
             BlockContainerBuilder::new(context, &anonymous_info, self.text_decoration_line);
 
@@ -914,22 +921,13 @@ where
             }
         }
 
-        let tag = Tag::new_pseudo(
-            self.info.node.opaque(),
-            Some(PseudoElement::ServoAnonymousTableCell),
-        );
-        let base_fragment_info = BaseFragmentInfo {
-            tag,
-            flags: FragmentFlags::empty(),
-        };
-
         let block_container = builder.finish();
         self.table_traversal.builder.add_cell(TableSlotCell {
             contents: BlockFormattingContext::from_block_container(block_container),
             colspan: 1,
             rowspan: 1,
             style: anonymous_info.style,
-            base_fragment_info,
+            base_fragment_info: BaseFragmentInfo::anonymous(),
         });
     }
 }
@@ -965,9 +963,12 @@ where
                     // 65534 and `colspan` to 1000, so we also enforce the same limits
                     // when dealing with arbitrary DOM elements (perhaps created via
                     // script).
-                    let node = info.node.to_threadsafe();
-                    let rowspan = (node.get_rowspan().unwrap_or(1) as usize).min(65534);
-                    let colspan = (node.get_colspan().unwrap_or(1) as usize).min(1000);
+                    let (rowspan, colspan) = info.node.map_or((1, 1), |node| {
+                        let node = 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;
+                        (rowspan, colspan)
+                    });
 
                     let contents = match contents.try_into() {
                         Ok(non_replaced_contents) => {
diff --git a/components/layout_2020/table/mod.rs b/components/layout_2020/table/mod.rs
index c8d620f2cc4..f3f0d676e7b 100644
--- a/components/layout_2020/table/mod.rs
+++ b/components/layout_2020/table/mod.rs
@@ -198,7 +198,7 @@ impl TableSlotCell {
 
     /// Get the node id of this cell's [`BaseFragmentInfo`]. This is used for unit tests.
     pub fn node_id(&self) -> usize {
-        self.base_fragment_info.tag.node.0
+        self.base_fragment_info.tag.map_or(0, |tag| tag.node.0)
     }
 }
author	Martin Robinson <mrobinson@igalia.com>	2024-03-09 10:13:19 +0100
committer	GitHub <noreply@github.com>	2024-03-09 09:13:19 +0000
commit	1f23ec2b27789c356a6283d9005079b6e9b1e66b (patch)
tree	df5256c59df043d12dd2630e2a3cf90e0a62672c
parent	55f908653f6fb02c344459319a7ca87487cfa4bf (diff)
download	servo-1f23ec2b27789c356a6283d9005079b6e9b1e66b.tar.gz servo-1f23ec2b27789c356a6283d9005079b6e9b1e66b.zip