Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Mar 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 27 additions & 6 deletions components/layout_2020/dom_traversal.rs
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all callers of this use new_anonymous?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to keep it for a couple callers which expect things to be inherited or which are creating list markers. There's a larger refactor we should do to clean this up and we need to implement the ::marker pseudo, but these are pretty intrusive things that should be done in a larger project.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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