Skip to content

Commit

Permalink
Remove unnecessary ClipDisplayItems in box_.rs
Browse files Browse the repository at this point in the history
For ScannedTextBox and ImageBox, the ClipDisplayItem's child_list is
currently always empty.

ClipDisplayItem is used to implement overflow hidden and should only be
created for block containers, as per
http://www.w3.org/TR/CSS2/visufx.html#propdef-overflow

Take care of the case when a BlockFlow has no children display items -
like an ImageBox with "display: block".
  • Loading branch information
pradeep90 committed Feb 4, 2014
1 parent 73d0cea commit 35a869d
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 54 deletions.
26 changes: 1 addition & 25 deletions src/components/main/layout/box_.rs
Expand Up @@ -967,18 +967,6 @@ impl Box {
match self.specific {
UnscannedTextBox(_) => fail!("Shouldn't see unscanned boxes here."),
ScannedTextBox(ref text_box) => {
lists.with_mut(|lists| {
let item = ~ClipDisplayItem {
base: BaseDisplayItem {
bounds: absolute_box_bounds,
extra: ExtraDisplayListData::new(self),
},
child_list: ~[],
need_clip: false
};
lists.lists[index].append_item(ClipDisplayItemClass(item));
});

let text_color = self.style().Color.color.to_gfx_color();

// Set the various text display item flags.
Expand All @@ -998,7 +986,7 @@ impl Box {
text_flags.set_override_underline(flow_flags.flags.override_underline());
text_flags.set_override_overline(flow_flags.flags.override_overline());
text_flags.set_override_line_through(flow_flags.flags.override_line_through());

let mut bounds = absolute_box_bounds.clone();
bounds.origin.x = bounds.origin.x + self.noncontent_left()
+ self.noncontent_inline_left();
Expand Down Expand Up @@ -1104,18 +1092,6 @@ impl Box {
});
},
ImageBox(ref image_box) => {
lists.with_mut(|lists| {
let item = ~ClipDisplayItem {
base: BaseDisplayItem {
bounds: absolute_box_bounds,
extra: ExtraDisplayListData::new(self),
},
child_list: ~[],
need_clip: false
};
lists.lists[index].append_item(ClipDisplayItemClass(item));
});

let mut image_ref = image_box.image.borrow_mut();
let mut bounds = absolute_box_bounds.clone();
bounds.origin.x = bounds.origin.x + self.noncontent_left()
Expand Down
77 changes: 48 additions & 29 deletions src/components/main/layout/flow.rs
Expand Up @@ -167,6 +167,9 @@ pub trait ImmutableFlowUtils {
/// Returns the number of children that this flow possesses.
fn child_count(self) -> uint;

/// Return true if this flow is a Block Container.
fn is_block_container(self) -> bool;

/// Returns true if this flow is a block flow, an inline flow, or a float flow.
fn starts_block_flow(self) -> bool;

Expand Down Expand Up @@ -604,6 +607,23 @@ impl<'a> ImmutableFlowUtils for &'a Flow {
base(self).children.len()
}

/// Return true if this flow is a Block Container.
///
/// Except for table boxes and replaced elements, block-level boxes (`BlockFlow`) are
/// also block container boxes.
/// Non-replaced inline blocks and non-replaced table cells are also block
/// containers.
fn is_block_container(self) -> bool {
match self.class() {
// TODO: Change this when inline-blocks are supported.
InlineFlowClass => false,
BlockFlowClass => {
// FIXME: Actually check the type of the node
self.child_count() != 0
}
}
}

/// Returns true if this flow is a block flow, an inline-block flow, or a float flow.
fn starts_block_flow(self) -> bool {
match self.class() {
Expand Down Expand Up @@ -720,37 +740,36 @@ impl<'a> MutableFlowUtils for &'a mut Flow {
return true;
}

let mut child_lists = DisplayListCollection::new();
child_lists.add_list(DisplayList::new());
let child_lists = RefCell::new(child_lists);
for kid in child_iter(self) {
kid.build_display_lists(builder, dirty, 0u, &child_lists);
}

let mut child_lists = Some(child_lists.unwrap());
// Find parent ClipDisplayItemClass and push all child display items
// under it
// FIXME: Once we have children for InlineFlow, this might lead to
// children display items being pushed under the ClipDisplayItemClass
// created by the last box of the InlineFlow. Fix the logic.
lists.with_mut(|lists| {
let mut child_lists = child_lists.take_unwrap();
let result = lists.lists[index].list.mut_rev_iter().position(|item| {
match *item {
ClipDisplayItemClass(ref mut item) => {
item.child_list.push_all_move(child_lists.lists.shift().list);
true
},
_ => false,
}
});

if result.is_none() {
fail!("fail to find parent item");
if self.is_block_container() {
let mut child_lists = DisplayListCollection::new();
child_lists.add_list(DisplayList::new());
let child_lists = RefCell::new(child_lists);
for kid in child_iter(self) {
kid.build_display_lists(builder, dirty, 0u, &child_lists);
}

lists.lists.push_all_move(child_lists.lists);
});
let mut child_lists = Some(child_lists.unwrap());
// Find parent ClipDisplayItemClass and push all child display items
// under it
lists.with_mut(|lists| {
let mut child_lists = child_lists.take_unwrap();
let result = lists.lists[index].list.mut_rev_iter().position(|item| {
match *item {
ClipDisplayItemClass(ref mut item) => {
item.child_list.push_all_move(child_lists.lists.shift().list);
true
},
_ => false,
}
});

if result.is_none() {
fail!("fail to find parent item");
}

lists.lists.push_all_move(child_lists.lists);
});
}
true
}

Expand Down

5 comments on commit 35a869d

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

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

saw approval from larsbergstrom, larsbergstrom
at pradeep90@35a869d

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

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

merging pradeep90/servo/stacking-context = 35a869d into auto

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

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

pradeep90/servo/stacking-context = 35a869d merged ok, testing candidate = 5b93bc7

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

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

@bors-servo
Copy link
Contributor

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 5b93bc7

Please sign in to comment.