Skip to content

Commit

Permalink
layout_2020: Use ArcRefCell to track hoisted fragments
Browse files Browse the repository at this point in the history 
This avoids the use of lookup tables for containing blocks when
constructing the stacking context tree.

This seems to catch some laid-out hoisted fragments that were otherwise
dropped in the previous design. The changes cause one new test to pass
and one to fail. Visual examination of the failing tests reveals that
it's a progression (list markers are appearing when they were previously
not rendered).
  • Loading branch information
@mrobinson
mrobinson committed Mar 27, 2020
1 parent ebaa73d commit 19f4b70
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 156 deletions.
91 changes: 38 additions & 53 deletions components/layout_2020/display_list/stacking_context.rs
View file
Expand Up @@ -9,10 +9,8 @@ use crate::fragments::{
AbsoluteOrFixedPositionedFragment, AnonymousFragment, BoxFragment, Fragment,
};
use crate::geom::PhysicalRect;
use crate::positioned::HoistedFragmentId;
use crate::style_ext::ComputedValuesExt;
use euclid::default::Rect;
use fnv::FnvHashMap;
use gfx_traits::{combine_id_with_fragment_type, FragmentType};
use servo_arc::Arc as ServoArc;
use std::cmp::Ordering;
Expand All @@ -37,30 +35,13 @@ pub(crate) struct ContainingBlock {

/// The physical rect of this containing block.
rect: PhysicalRect<Length>,

/// Fragments for positioned descendants (including direct children) that were
/// hoisted into this containing block. They have hashed based on the
/// HoistedFragmentId that is generated during hoisting.
hoisted_children: FnvHashMap<HoistedFragmentId, ArcRefCell<Fragment>>,
}

impl ContainingBlock {
pub(crate) fn new(
rect: &PhysicalRect<Length>,
space_and_clip: wr::SpaceAndClipInfo,
children: &[ArcRefCell<Fragment>],
) -> Self {
let mut hoisted_children = FnvHashMap::default();
for child in children {
if let Some(hoisted_fragment_id) = child.borrow().hoisted_fragment_id() {
hoisted_children.insert(*hoisted_fragment_id, child.clone());
}
}

pub(crate) fn new(rect: &PhysicalRect<Length>, space_and_clip: wr::SpaceAndClipInfo) -> Self {
ContainingBlock {
space_and_clip,
rect: *rect,
hoisted_children,
}
}
}
Expand Down Expand Up @@ -333,12 +314,14 @@ impl Fragment {
stacking_context: &mut StackingContext,
mode: StackingContextBuildMode,
) {
if mode == StackingContextBuildMode::SkipHoisted && self.is_hoisted() {
return;
}

match self {
Fragment::Box(fragment) => {
if mode == StackingContextBuildMode::SkipHoisted &&
fragment.style.clone_position().is_absolutely_positioned()
{
return;
}

fragment.build_stacking_context_tree(
fragment_ref,
builder,
Expand Down Expand Up @@ -417,7 +400,7 @@ impl BoxFragment {
}

let new_containing_block =
ContainingBlock::new(padding_rect, builder.current_space_and_clip, &self.children);
ContainingBlock::new(padding_rect, builder.current_space_and_clip);

if self
.style
Expand Down Expand Up @@ -750,37 +733,39 @@ impl AbsoluteOrFixedPositionedFragment {
containing_block_info: &ContainingBlockInfo,
stacking_context: &mut StackingContext,
) {
let mut build_for_containing_block = |containing_block: &ContainingBlock| {
let hoisted_child = match containing_block.hoisted_children.get(&self.0) {
Some(hoisted_child) => hoisted_child,
None => return false,
};

builder.clipping_and_scrolling_scope(|builder| {
let mut new_containing_block_info = containing_block_info.clone();
new_containing_block_info.rect = containing_block.rect;
builder.current_space_and_clip = containing_block.space_and_clip;
hoisted_child.borrow().build_stacking_context_tree(
hoisted_child,
builder,
&new_containing_block_info,
stacking_context,
StackingContextBuildMode::IncludeHoisted,
);
});
let hoisted_fragment = self.hoisted_fragment.borrow();
let fragment_ref = match hoisted_fragment.as_ref() {
Some(fragment_ref) => fragment_ref,
None => {
warn!("Found hoisted box with missing fragment.");
return;
},
};

return true;
let containing_block = match self.position {
ComputedPosition::Fixed => &containing_block_info.containing_block_for_all_descendants,
ComputedPosition::Absolute => containing_block_info
.nearest_containing_block
.as_ref()
.unwrap_or(&containing_block_info.containing_block_for_all_descendants),
ComputedPosition::Static | ComputedPosition::Relative => unreachable!(
"Found an AbsoluteOrFixedPositionedFragment for a \
non-absolutely or fixed position fragment."
),
};

if let Some(containing_block) = containing_block_info.nearest_containing_block.as_ref() {
if build_for_containing_block(containing_block) {
return;
}
}
builder.clipping_and_scrolling_scope(|builder| {
let mut new_containing_block_info = containing_block_info.clone();
new_containing_block_info.rect = containing_block.rect;
builder.current_space_and_clip = containing_block.space_and_clip;

if !build_for_containing_block(&containing_block_info.containing_block_for_all_descendants)
{
warn!("Could not find containing block of hoisted positioned child!");
}
fragment_ref.borrow().build_stacking_context_tree(
fragment_ref,
builder,
&new_containing_block_info,
stacking_context,
StackingContextBuildMode::IncludeHoisted,
);
});
}
}
14 changes: 7 additions & 7 deletions components/layout_2020/flow/inline.rs
View file
Expand Up @@ -304,12 +304,15 @@ impl InlineFormattingContext {
},
};
let hoisted_box = box_.clone().to_hoisted(initial_start_corner, tree_rank);
let hoisted_fragment_id = hoisted_box.fragment_id;
let hoisted_fragment = hoisted_box.fragment.clone();
ifc.push_hoisted_box_to_positioning_context(hoisted_box);
ifc.current_nesting_level.fragments_so_far.push(
Fragment::AbsoluteOrFixedPositioned(AbsoluteOrFixedPositionedFragment(
hoisted_fragment_id,
)),
Fragment::AbsoluteOrFixedPositioned(
AbsoluteOrFixedPositionedFragment {
hoisted_fragment,
position: box_.contents.style.clone_position(),
},
),
);
},
InlineLevelBox::OutOfFlowFloatBox(_box_) => {
Expand Down Expand Up @@ -492,7 +495,6 @@ impl<'box_tree> PartialInlineBoxFragment<'box_tree> {
self.border.clone(),
self.margin.clone(),
CollapsedBlockMargins::zero(),
None, // hoisted_fragment_id
);
let last_fragment = self.last_box_tree_fragment && !at_line_break;
if last_fragment {
Expand Down Expand Up @@ -560,7 +562,6 @@ fn layout_atomic(
border,
margin,
CollapsedBlockMargins::zero(),
None, // hoisted_fragment_id
)
},
Err(non_replaced) => {
Expand Down Expand Up @@ -636,7 +637,6 @@ fn layout_atomic(
border,
margin,
CollapsedBlockMargins::zero(),
None, // hoisted_fragment_id
)
},
};
Expand Down
15 changes: 7 additions & 8 deletions components/layout_2020/flow/mod.rs
View file
Expand Up @@ -315,12 +315,13 @@ impl BlockLevelBox {
))
},
BlockLevelBox::OutOfFlowAbsolutelyPositionedBox(box_) => {
let hoisted_fragment = box_.clone().to_hoisted(Vec2::zero(), tree_rank);
let hoisted_fragment_id = hoisted_fragment.fragment_id.clone();
positioning_context.push(hoisted_fragment);
Fragment::AbsoluteOrFixedPositioned(AbsoluteOrFixedPositionedFragment(
hoisted_fragment_id,
))
let hoisted_box = box_.clone().to_hoisted(Vec2::zero(), tree_rank);
let hoisted_fragment = hoisted_box.fragment.clone();
positioning_context.push(hoisted_box);
Fragment::AbsoluteOrFixedPositioned(AbsoluteOrFixedPositionedFragment {
hoisted_fragment,
position: box_.contents.style.clone_position(),
})
},
BlockLevelBox::OutOfFlowFloatBox(_box_) => {
// FIXME: call layout_maybe_position_relative_fragment here
Expand Down Expand Up @@ -501,7 +502,6 @@ fn layout_in_flow_non_replaced_block_level(
border,
margin,
block_margins_collapsed_with_children,
None, // hoisted_fragment_id
)
}

Expand Down Expand Up @@ -553,7 +553,6 @@ fn layout_in_flow_replaced_block_level<'a>(
border,
margin,
block_margins_collapsed_with_children,
None, // hoisted_fragment_id
)
}

Expand Down
56 changes: 27 additions & 29 deletions components/layout_2020/flow/root.rs
View file
Expand Up @@ -147,48 +147,47 @@ impl BoxTreeRoot {
let dummy_tree_rank = 0;
let mut positioning_context =
PositioningContext::new_for_containing_block_for_all_descendants();
let mut independent_layout = self.0.layout(
let independent_layout = self.0.layout(
layout_context,
&mut positioning_context,
&(&initial_containing_block).into(),
dummy_tree_rank,
);

let mut children = independent_layout
.fragments
.into_iter()
.map(|fragment| ArcRefCell::new(fragment))
.collect();
positioning_context.layout_initial_containing_block_children(
layout_context,
&initial_containing_block,
&mut independent_layout.fragments,
&mut children,
);

let scrollable_overflow =
independent_layout
.fragments
.iter()
.fold(PhysicalRect::zero(), |acc, child| {
let child_overflow = child.scrollable_overflow(&physical_containing_block);
let scrollable_overflow = children.iter().fold(PhysicalRect::zero(), |acc, child| {
let child_overflow = child
.borrow()
.scrollable_overflow(&physical_containing_block);

// https://drafts.csswg.org/css-overflow/#scrolling-direction
// We want to clip scrollable overflow on box-start and inline-start
// sides of the scroll container.
//
// FIXME(mrobinson, bug 25564): This should take into account writing
// mode.
let child_overflow = PhysicalRect::new(
euclid::Point2D::zero(),
euclid::Size2D::new(
child_overflow.size.width + child_overflow.origin.x,
child_overflow.size.height + child_overflow.origin.y,
),
);
acc.union(&child_overflow)
});
// https://drafts.csswg.org/css-overflow/#scrolling-direction
// We want to clip scrollable overflow on box-start and inline-start
// sides of the scroll container.
//
// FIXME(mrobinson, bug 25564): This should take into account writing
// mode.
let child_overflow = PhysicalRect::new(
euclid::Point2D::zero(),
euclid::Size2D::new(
child_overflow.size.width + child_overflow.origin.x,
child_overflow.size.height + child_overflow.origin.y,
),
);
acc.union(&child_overflow)
});

FragmentTreeRoot {
children: independent_layout
.fragments
.into_iter()
.map(|fragment| ArcRefCell::new(fragment))
.collect(),
children,
scrollable_overflow,
initial_containing_block: physical_containing_block,
}
Expand All @@ -206,7 +205,6 @@ impl FragmentTreeRoot {
containing_block_for_all_descendants: ContainingBlock::new(
&self.initial_containing_block,
stacking_context_builder.current_space_and_clip,
&self.children,
),
};

Expand Down

0 comments on commit 19f4b70

Please sign in to comment.