Skip to content

Commit

Permalink
layout: Do not inherit node and fragment flags in anonymous boxes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mrobinson
mrobinson committed Mar 9, 2024
1 parent 55f9086 commit b971d79
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 38 deletions.
33 changes: 27 additions & 6 deletions components/layout_2020/dom_traversal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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>,
}
Expand All @@ -39,22 +40,30 @@ impl<Node> NodeAndStyleInfo<Node> {
style: ServoArc<ComputedValues>,
) -> Self {
Self {
node,
node: Some(node),
pseudo_element_type: Some(pseudo_element_type),
style,
}
}

pub(crate) fn new(node: Node, style: ServoArc<ComputedValues>) -> Self {
Self {
node,
node: Some(node),
pseudo_element_type: None,
style,
}
}
}

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(),
Expand All @@ -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
Expand All @@ -86,7 +100,7 @@ where
};

Self {
tag: Tag::new_pseudo(threadsafe_node.opaque(), pseudo),
tag: Some(Tag::new_pseudo(threadsafe_node.opaque(), pseudo)),
flags,
}
}
Expand Down Expand Up @@ -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)
},
Expand Down
4 changes: 1 addition & 3 deletions components/layout_2020/flexbox/construct.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
4 changes: 2 additions & 2 deletions components/layout_2020/flow/construct.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
15 changes: 11 additions & 4 deletions components/layout_2020/fragment_tree/base_fragment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
}
}
Expand All @@ -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,
}
Expand Down
10 changes: 9 additions & 1 deletion components/layout_2020/lists.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()),
]),
Expand Down
43 changes: 22 additions & 21 deletions components/layout_2020/table/construct.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);

Expand All @@ -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(),
});
}
}
Expand Down Expand Up @@ -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) => {
Expand Down
2 changes: 1 addition & 1 deletion components/layout_2020/table/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down

0 comments on commit b971d79

Please sign in to comment.