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.

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

Implement sequential fallback to float speculation #13401

Merged
merged 3 commits into from Sep 29, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

Implement sequential fallback to float speculation

Fixes #13284
Fixes #13223
  • Loading branch information
notriddle committed Sep 27, 2016
commit fe2018682b2718b27031a573a738fe01d49001b1
@@ -52,6 +52,7 @@ use model::{CollapsibleMargins, MaybeAuto, specified, specified_or_none};
use rustc_serialize::{Encodable, Encoder};
use script_layout_interface::restyle_damage::{BUBBLE_ISIZES, REFLOW, REFLOW_OUT_OF_FLOW};
use script_layout_interface::restyle_damage::REPOSITION;
use sequential;
use std::cmp::{max, min};
use std::fmt;
use std::sync::Arc;
@@ -781,6 +782,7 @@ impl BlockFlow {
self.base.debug_id());

let mut break_at = None;
let content_box = self.fragment.content_box();
if self.base.restyle_damage.contains(REFLOW) {
self.determine_if_layer_needed();

@@ -830,7 +832,8 @@ impl BlockFlow {
kid.place_float_if_applicable();
if !flow::base(kid).flags.is_float() {
kid.assign_block_size_for_inorder_child_if_necessary(layout_context,
thread_id);
thread_id,
content_box);
}

// Skip the collapsing and float processing for absolute flow kids and continue
@@ -881,7 +884,8 @@ impl BlockFlow {
// Lay the child out if this was an in-order traversal.
let need_to_process_child_floats =
kid.assign_block_size_for_inorder_child_if_necessary(layout_context,
thread_id);
thread_id,
content_box);

if !had_children_with_clearance &&
floats.is_present() &&
@@ -1062,7 +1066,9 @@ impl BlockFlow {
// necessary.
let thread_id = self.base.thread_id;
for kid in self.base.child_iter_mut() {
kid.assign_block_size_for_inorder_child_if_necessary(layout_context, thread_id);
kid.assign_block_size_for_inorder_child_if_necessary(layout_context,
thread_id,
content_box);
}
}

@@ -1457,32 +1463,104 @@ impl BlockFlow {
/// on the floats we could see at the time of inline-size assignment. The job of this function,
/// therefore, is not only to assign the final size but also to perform the layout again for
/// this block formatting context if our speculation was wrong.
///
/// FIXME(pcwalton): This code is not incremental-reflow-safe (i.e. not idempotent).
fn assign_inline_position_for_formatting_context(&mut self) {
fn assign_inline_position_for_formatting_context<'a>(&mut self,
layout_context: &'a LayoutContext<'a>,
content_box: LogicalRect<Au>) {
debug_assert!(self.formatting_context_type() != FormattingContextType::None);

if !self.base.restyle_damage.intersects(REFLOW_OUT_OF_FLOW | REFLOW) {
return
}

let info = PlacementInfo {
size: self.fragment.border_box.size.convert(self.fragment.style.writing_mode,
self.base.floats.writing_mode),
ceiling: self.base.position.start.b,
max_inline_size: MAX_AU,
kind: FloatKind::Left,
};
// We do this first to avoid recomputing our inline size when we propagate it.
self.base.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);
self.fragment.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);

// Offset our position by whatever displacement is needed to not impact the floats.
let rect = self.base.floats.place_between_floats(&info);
self.base.position.start.i = self.base.position.start.i + rect.start.i;
// The code below would completely wreck the layout if run on a flex item, however:
// * Flex items are always the children of flex containers.
// * Flex containers only contain flex items.
// * Floats cannot intrude into flex containers.
// * Floats cannot escape flex items.
// * Flex items cannot also be floats.
// Therefore, a flex item cannot be impacted by a float.
// See also: https://www.w3.org/TR/css-flexbox-1/#flex-containers
// This line is not just an optimization. It's also needed for correctness.

This comment has been minimized.

@pcwalton

pcwalton Sep 27, 2016

Contributor

Why?

This comment has been minimized.

@notriddle

notriddle Sep 27, 2016

Author Contributor

Because if you remove it, it tries to run the float fixup on flex items, breaking the flexbox test suite.

Is there an easy way for me to assert that the current flow isn't a flex item?

This comment has been minimized.

@stshine

stshine Sep 27, 2016

Contributor

You can use self.is_flex()

This comment has been minimized.

@notriddle

notriddle Sep 27, 2016

Author Contributor

Done. Thanks!

if !self.base.might_have_floats_in() {
return
}

// Compute the available space for us, based on the actual floats.
let rect = self.base.floats.available_rect(
self.base.position.start.b,
self.fragment.border_box.size.block,
content_box.size.inline
);
let available_inline_size = if let Some(rect) = rect {
// Offset our position by whatever displacement is needed to not impact the floats.
// Also, account for margins sliding behind floats.
let inline_offset = if self.fragment.margin.inline_start < rect.start.i {
// Do not do anything for negative margins; those are handled separately.
rect.start.i - max(Au(0), self.fragment.margin.inline_start)
} else {
Au(0)
};
self.base.position.start.i = content_box.start.i + inline_offset;
// Handle the end margin sliding behind the float.
let end = content_box.size.inline - rect.start.i - rect.size.inline;
let inline_end_offset = if self.fragment.margin.inline_end < end {
end - max(Au(0), self.fragment.margin.inline_end)
} else {
Au(0)
};
content_box.size.inline - inline_offset - inline_end_offset
} else {
content_box.size.inline
} - self.fragment.margin.inline_start_end();
let max_inline_size = specified_or_none(
self.fragment.style().max_inline_size(),
self.base.block_container_inline_size
).unwrap_or(MAX_AU);
let min_inline_size = specified(
self.fragment.style().min_inline_size(),
self.base.block_container_inline_size
);
let specified_inline_size = self.fragment.style().content_inline_size();
let container_size = self.base.block_container_inline_size;
let inline_size =
if let MaybeAuto::Specified(size) = MaybeAuto::from_style(specified_inline_size,
container_size) {
match self.fragment.style().get_position().box_sizing {
box_sizing::T::border_box => size,
box_sizing::T::content_box =>
size + self.fragment.border_padding.inline_start_end(),
}
} else if available_inline_size > max_inline_size {
max_inline_size
} else if available_inline_size < min_inline_size {
min_inline_size
} else {
available_inline_size
};

This comment has been minimized.

@pcwalton

pcwalton Sep 27, 2016

Contributor

Do we not have a clamp function for this?

This comment has been minimized.

@notriddle

notriddle Sep 27, 2016

Author Contributor

Not that I could find. I can add one, though.

This comment has been minimized.

@notriddle

notriddle Sep 27, 2016

Author Contributor

Done.

self.base.position.size.inline = inline_size + self.fragment.margin.inline_start_end();

// TODO(pcwalton): If the inline-size of this flow is different from the size we estimated
// earlier, lay it out again.
// If float speculation failed, fixup our layout, and re-layout all the children.
if self.fragment.margin_box_inline_size() != self.base.position.size.inline {

This comment has been minimized.

@pcwalton

pcwalton Sep 24, 2016

Contributor

Could you add a debug!() log message here? For performance reasons, it's nice to know when we hit this case.

This comment has been minimized.

@notriddle

notriddle Sep 24, 2016

Author Contributor

Done.

debug!("assign_inline_position_for_formatting_context: float speculation failed");
// Fix-up our own layout.
// We can't just traverse_flow_tree_preorder ourself, because that would re-run
// float speculation, instead of acting on the actual results.
self.fragment.border_box.size.inline = inline_size;
// Assign final-final inline sizes on all our children.
self.assign_inline_sizes(&layout_context.shared.style_context);
// Re-run layout on our children.
for child in flow::mut_base(self).children.iter_mut() {
sequential::traverse_flow_tree_preorder(child, layout_context.shared);
}
// Assign our final-final block size.
self.assign_block_size(layout_context);
}

self.base.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);
self.fragment.restyle_damage.remove(REFLOW_OUT_OF_FLOW | REFLOW);
debug_assert_eq!(self.fragment.margin_box_inline_size(), self.base.position.size.inline);
}

fn is_inline_block(&self) -> bool {
@@ -1792,15 +1870,16 @@ impl Flow for BlockFlow {

fn assign_block_size_for_inorder_child_if_necessary<'a>(&mut self,
layout_context: &'a LayoutContext<'a>,
parent_thread_id: u8)
parent_thread_id: u8,
content_box: LogicalRect<Au>)
-> bool {
if self.base.flags.is_float() {
return false
}

let is_formatting_context = self.formatting_context_type() != FormattingContextType::None;
if !self.base.flags.contains(IS_ABSOLUTELY_POSITIONED) && is_formatting_context {
self.assign_inline_position_for_formatting_context();
self.assign_inline_position_for_formatting_context(layout_context, content_box);
}

if (self as &Flow).floats_might_flow_through() {
@@ -241,7 +241,8 @@ pub trait Flow: fmt::Debug + Sync + Send + 'static {
/// it as laid out by its parent.
fn assign_block_size_for_inorder_child_if_necessary<'a>(&mut self,
layout_context: &'a LayoutContext<'a>,
parent_thread_id: u8)
parent_thread_id: u8,
_content_box: LogicalRect<Au>)
-> bool {
let might_have_floats_in_or_out = base(self).might_have_floats_in() ||
base(self).might_have_floats_out();
@@ -1481,7 +1481,10 @@ impl Flow for InlineFlow {
flow::base(kid).flags.is_float() {
continue
}
kid.assign_block_size_for_inorder_child_if_necessary(layout_context, thread_id);
let content_box = flow::base(kid).position;
kid.assign_block_size_for_inorder_child_if_necessary(layout_context,
thread_id,
content_box);
}

if self.contains_positioned_fragments() {
@@ -112,11 +112,15 @@ impl TableRowFlow {
// all cells).
let mut max_block_size = Au(0);
let thread_id = self.block_flow.base.thread_id;
let content_box = self.block_flow.base.position
- self.block_flow.fragment.border_padding
- self.block_flow.fragment.margin;
for kid in self.block_flow.base.child_iter_mut() {
kid.place_float_if_applicable();
if !flow::base(kid).flags.is_float() {
kid.assign_block_size_for_inorder_child_if_necessary(layout_context,
thread_id);
thread_id,
content_box);
}

{
@@ -32,7 +32,7 @@ use std::ops::Add;
use std::sync::Arc;
use style::computed_values::{border_collapse, table_layout};
use style::context::SharedStyleContext;
use style::logical_geometry::LogicalSize;
use style::logical_geometry::{LogicalRect, LogicalSize};
use style::properties::ServoComputedValues;
use style::values::CSSFloat;
use style::values::computed::LengthOrPercentageOrAuto;
@@ -423,10 +423,12 @@ impl Flow for TableWrapperFlow {

fn assign_block_size_for_inorder_child_if_necessary<'a>(&mut self,
layout_context: &'a LayoutContext<'a>,
parent_thread_id: u8)
parent_thread_id: u8,
content_box: LogicalRect<Au>)
-> bool {
self.block_flow.assign_block_size_for_inorder_child_if_necessary(layout_context,
parent_thread_id)
parent_thread_id,
content_box)
}

fn update_late_computed_inline_position_if_necessary(&mut self, inline_position: Au) {

This file was deleted.

This file was deleted.

@@ -1592,6 +1592,18 @@
"url": "/_mozilla/css/float_clearance_intrinsic_width_a.html"
}
],
"css/float_cleared_with_just_height.html": [
{
"path": "css/float_cleared_with_just_height.html",
"references": [
[
"/_mozilla/css/float_cleared_with_just_height_ref.html",
"=="
]
],
"url": "/_mozilla/css/float_cleared_with_just_height.html"
}
],
"css/float_intrinsic_height.html": [
{
"path": "css/float_intrinsic_height.html",
"url": "/_mozilla/css/float_clearance_intrinsic_width_a.html"
}
],
"css/float_cleared_with_just_height.html": [
{
"path": "css/float_cleared_with_just_height.html",
"references": [
[
"/_mozilla/css/float_cleared_with_just_height_ref.html",
"=="
]
],
"url": "/_mozilla/css/float_cleared_with_just_height.html"
}
],
"css/float_intrinsic_height.html": [
{
"path": "css/float_intrinsic_height.html",
@@ -0,0 +1,25 @@
<!doctype html>
<meta charset="utf-8">
<title>floats can be cleared with just height</title>
<link rel="match" href="float_cleared_with_just_height_ref.html">
<style>
.a {
height: 20px;
}
.b {
height: 10px;
width: 50px;
float: right;
}
.c {
border: solid 1px #000;
background: red;
height: 10px;
overflow: auto
}
</style>
<div class=b></div>
<div class=a></div>
<div class=b></div>
<div class=a></div>
<div class=c></div>
@@ -0,0 +1,17 @@
<!doctype html>
<meta charset="utf-8">
<title>REFERENCE: floats can be cleared with just height</title>
<style>
.a {
height: 20px;
}
.c {
border: solid 1px #000;
background: red;
height: 10px;
overflow: auto
}
</style>
<div class=a></div>
<div class=a></div>
<div class=c></div>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.