From b416bb3aa7e3da3a33dc0c115f1f7619914f382f Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 12 Feb 2018 14:58:15 -0800 Subject: [PATCH 01/20] Add get_column_styles for getting column structure and styles for a table --- components/layout/flow.rs | 6 +++++ components/layout/fragment.rs | 10 +++++++++ components/layout/table.rs | 35 +++++++++++++++++++++++++++++ components/layout/table_colgroup.rs | 13 +++++------ 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/components/layout/flow.rs b/components/layout/flow.rs index 3f2b64827af..30877defa43 100644 --- a/components/layout/flow.rs +++ b/components/layout/flow.rs @@ -171,6 +171,12 @@ pub trait Flow: HasBaseFlow + fmt::Debug + Sync + Send + 'static { panic!("called as_mut_table_colgroup() on a non-tablecolgroup flow") } + /// If this is a table colgroup flow, returns the underlying object. Fails + /// otherwise. + fn as_table_colgroup(&self) -> &TableColGroupFlow { + panic!("called as_table_colgroup() on a non-tablecolgroup flow") + } + /// If this is a table rowgroup flow, returns the underlying object, borrowed mutably. Fails /// otherwise. fn as_mut_table_rowgroup(&mut self) -> &mut TableRowGroupFlow { diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index e03de77a6cd..dc09280315a 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -1408,6 +1408,16 @@ impl Fragment { } } + /// If this is a Column fragment, get the col span + /// + /// Panics for non-column fragments + pub fn column_span(&self) -> u32 { + match self.specific { + SpecificFragmentInfo::TableColumn(col_fragment) => max(col_fragment.span, 1), + _ => panic!("non-table-column fragment inside table column?!"), + } + } + /// Returns true if this element can be split. This is true for text fragments, unless /// `white-space: pre` or `white-space: nowrap` is set. pub fn can_split(&self) -> bool { diff --git a/components/layout/table.rs b/components/layout/table.rs index 5068827c0d7..54e9b73d2fe 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -203,6 +203,34 @@ impl TableFlow { } self.spacing().horizontal() * (num_columns as i32 + 1) } + + fn column_styles(&self) -> Vec { + let mut styles = vec![]; + for group in self.block_flow.base.child_iter() + .filter(|kid| kid.is_table_colgroup()) { + // XXXManishearth these as_foo methods should return options + // so that we can filter_map + let group = group.as_table_colgroup(); + let colgroup_style = group.fragment.as_ref().map(|f| f.style()); + + // The colgroup's span attribute is only relevant when + // it has no children + // https://html.spec.whatwg.org/multipage/#forming-a-table + if group.cols.is_empty() { + let span = group.fragment.as_ref().map(|f| f.column_span()).unwrap_or(1); + styles.push(ColumnStyle { span, colgroup_style, col_style: None }); + } else { + for col in &group.cols { + styles.push(ColumnStyle { + span: col.column_span(), + colgroup_style, + col_style: Some(col.style()), + }) + } + } + } + styles + } } impl Flow for TableFlow { @@ -529,6 +557,13 @@ impl Flow for TableFlow { } } +#[derive(Debug, Copy, Clone)] +struct ColumnStyle<'a> { + span: u32, + colgroup_style: Option<&'a ComputedValues>, + col_style: Option<&'a ComputedValues>, +} + impl fmt::Debug for TableFlow { /// Outputs a debugging string describing this table flow. fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { diff --git a/components/layout/table_colgroup.rs b/components/layout/table_colgroup.rs index a341a611404..cd1260098f1 100644 --- a/components/layout/table_colgroup.rs +++ b/components/layout/table_colgroup.rs @@ -11,9 +11,8 @@ use context::LayoutContext; use display_list::{DisplayListBuildState, StackingContextCollectionState}; use euclid::Point2D; use flow::{BaseFlow, Flow, FlowClass, ForceNonfloatedFlag, OpaqueFlow}; -use fragment::{Fragment, FragmentBorderBoxIterator, Overflow, SpecificFragmentInfo}; +use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use layout_debug; -use std::cmp::max; use std::fmt; use style::logical_geometry::LogicalSize; use style::properties::ComputedValues; @@ -63,6 +62,10 @@ impl Flow for TableColGroupFlow { self } + fn as_table_colgroup(&self) -> &TableColGroupFlow { + self + } + fn bubble_inline_sizes(&mut self) { let _scope = layout_debug_scope!("table_colgroup::bubble_inline_sizes {:x}", self.base.debug_id()); @@ -70,11 +73,7 @@ impl Flow for TableColGroupFlow { for fragment in &self.cols { // Retrieve the specified value from the appropriate CSS property. let inline_size = fragment.style().content_inline_size(); - let span = match fragment.specific { - SpecificFragmentInfo::TableColumn(col_fragment) => max(col_fragment.span, 1), - _ => panic!("non-table-column fragment inside table column?!"), - }; - for _ in 0..span { + for _ in 0..fragment.column_span() { self.inline_sizes.push(inline_size) } } From 35be0c50f66da54de5a97480179912eb37323b75 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 12 Feb 2018 15:33:08 -0800 Subject: [PATCH 02/20] Add TableRowAndGroupIterator --- components/layout/table.rs | 48 +++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 54e9b73d2fe..10e041cb997 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -860,40 +860,43 @@ enum NextBlockCollapsedBorders<'a> { FromTable(CollapsedBorder), } -/// Iterator over all the rows of a table -struct TableRowIterator<'a> { +/// Iterator over all the rows of a table, which also +/// provides the Fragment for rowgroups if any +struct TableRowAndGroupIterator<'a> { kids: MutFlowListIterator<'a>, - grandkids: Option>, + group: Option<(&'a Fragment, MutFlowListIterator<'a>)> } -impl<'a> TableRowIterator<'a> { +impl<'a> TableRowAndGroupIterator<'a> { fn new(base: &'a mut BaseFlow) -> Self { - TableRowIterator { + TableRowAndGroupIterator { kids: base.child_iter_mut(), - grandkids: None, + group: None, } } } -impl<'a> Iterator for TableRowIterator<'a> { - type Item = &'a mut TableRowFlow; +impl<'a> Iterator for TableRowAndGroupIterator<'a> { + type Item = (Option<&'a Fragment>, &'a mut TableRowFlow); #[inline] fn next(&mut self) -> Option { // If we're inside a rowgroup, iterate through the rowgroup's children. - if let Some(ref mut grandkids) = self.grandkids { - if let Some(grandkid) = grandkids.next() { - return Some(grandkid.as_mut_table_row()) + if let Some(ref mut group) = self.group { + if let Some(grandkid) = group.1.next() { + return Some((Some(group.0), grandkid.as_mut_table_row())) } } // Otherwise, iterate through the table's children. - self.grandkids = None; + self.group = None; match self.kids.next() { Some(kid) => { if kid.is_table_rowgroup() { - self.grandkids = Some(kid.mut_base().child_iter_mut()); + let mut rowgroup = kid.as_mut_table_rowgroup(); + let iter = rowgroup.block_flow.base.child_iter_mut(); + self.group = Some((&rowgroup.block_flow.fragment, iter)); self.next() } else if kid.is_table_row() { - Some(kid.as_mut_table_row()) + Some((None, kid.as_mut_table_row())) } else { self.next() // Skip children that are not rows or rowgroups } @@ -902,3 +905,20 @@ impl<'a> Iterator for TableRowIterator<'a> { } } } + +/// Iterator over all the rows of a table +struct TableRowIterator<'a>(TableRowAndGroupIterator<'a>); + +impl<'a> TableRowIterator<'a> { + fn new(base: &'a mut BaseFlow) -> Self { + TableRowIterator(TableRowAndGroupIterator::new(base)) + } +} + +impl<'a> Iterator for TableRowIterator<'a> { + type Item = &'a mut TableRowFlow; + #[inline] + fn next(&mut self) -> Option { + self.0.next().map(|n| n.1) + } +} From 1dd5bed0310a9a1cceb1d5a98d9eb765616977f2 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 12 Feb 2018 16:25:15 -0800 Subject: [PATCH 03/20] Create TableCellStyleIterator --- components/layout/table.rs | 73 +++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 10e041cb997..508f7df7b84 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -19,12 +19,14 @@ use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use gfx_traits::print_tree::PrintTree; use layout_debug; use model::{IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto}; +use servo_arc::Arc; use std::cmp; use std::fmt; use style::computed_values::{border_collapse, border_spacing, table_layout}; use style::context::SharedStyleContext; use style::logical_geometry::LogicalSize; use style::properties::ComputedValues; +use style::properties::style_structs::Background; use style::servo::restyle_damage::ServoRestyleDamage; use style::values::CSSFloat; use style::values::computed::LengthOrPercentageOrAuto; @@ -211,20 +213,23 @@ impl TableFlow { // XXXManishearth these as_foo methods should return options // so that we can filter_map let group = group.as_table_colgroup(); - let colgroup_style = group.fragment.as_ref().map(|f| f.style()); + let colgroup_style = group.fragment.as_ref() + .map(|f| f.style().clone_background()); // The colgroup's span attribute is only relevant when // it has no children // https://html.spec.whatwg.org/multipage/#forming-a-table if group.cols.is_empty() { - let span = group.fragment.as_ref().map(|f| f.column_span()).unwrap_or(1); + let span = group.fragment.as_ref() + .map(|f| f.column_span()).unwrap_or(1); styles.push(ColumnStyle { span, colgroup_style, col_style: None }); } else { for col in &group.cols { + // XXXManishearth Arc-cloning colgroup_style is suboptimal styles.push(ColumnStyle { span: col.column_span(), - colgroup_style, - col_style: Some(col.style()), + colgroup_style: colgroup_style.clone(), + col_style: Some(col.style().clone_background()), }) } } @@ -557,11 +562,18 @@ impl Flow for TableFlow { } } -#[derive(Debug, Copy, Clone)] -struct ColumnStyle<'a> { +#[derive(Debug)] +// XXXManishearth We might be able to avoid the Arcs if +// the table is structured such that the columns always come +// first in the flow tree, at which point we can +// reuse the iterator that we use for colgroups +// for rows (and have no borrowing issues between +// holding on to both ColumnStyle<'table> and +// the rows) +struct ColumnStyle { span: u32, - colgroup_style: Option<&'a ComputedValues>, - col_style: Option<&'a ComputedValues>, + colgroup_style: Option>, + col_style: Option>, } impl fmt::Debug for TableFlow { @@ -922,3 +934,48 @@ impl<'a> Iterator for TableRowIterator<'a> { self.0.next().map(|n| n.1) } } + +/// An iterator over table cells, yielding all relevant style objects +/// for each cell +/// +/// Used for correctly handling table layers from +/// https://drafts.csswg.org/css2/tables.html#table-layers +struct TableCellStyleIterator<'table> { + column_styles: Vec, + row_iterator: TableRowAndGroupIterator<'table>, + row_info: Option>, + column_index: u32, + /// The index of the current column in column_styles + column_index_relative: u32, + /// In case of multispan columns, where we are in the + /// span of the current element + column_index_relative_offset: u32, +} + +struct TableCellStyleIteratorRowInfo<'table> { + row: &'table Fragment, + rowgroup: Option<&'table Fragment>, + cell_iterator: MutFlowListIterator<'table>, +} + +impl<'table> TableCellStyleIterator<'table> { + fn new(table: &'table mut TableFlow) -> Self { + let column_styles = table.column_styles(); + let mut row_iterator = TableRowAndGroupIterator::new(&mut table.block_flow.base); + let row_info = if let Some((group, row)) = row_iterator.next() { + Some(TableCellStyleIteratorRowInfo { + row: &row.block_flow.fragment, + rowgroup: group, + cell_iterator: row.block_flow.base.child_iter_mut() + }) + } else { + None + }; + TableCellStyleIterator { + column_styles, row_iterator, row_info, + column_index: 0, + column_index_relative: 0, + column_index_relative_offset: 0, + } + } +} From 6317c44ab407d08c035f5b3bb69431642cdc259c Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 12 Feb 2018 17:01:48 -0800 Subject: [PATCH 04/20] Add Iterator impl for TableCellStyleIterator --- components/layout/table.rs | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/components/layout/table.rs b/components/layout/table.rs index 508f7df7b84..f92b04ea551 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -30,6 +30,7 @@ use style::properties::style_structs::Background; use style::servo::restyle_damage::ServoRestyleDamage; use style::values::CSSFloat; use style::values::computed::LengthOrPercentageOrAuto; +use table_cell::TableCellFlow; use table_row::{self, CellIntrinsicInlineSize, CollapsedBorder, CollapsedBorderProvenance}; use table_row::TableRowFlow; use table_wrapper::TableLayout; @@ -979,3 +980,47 @@ impl<'table> TableCellStyleIterator<'table> { } } } + +struct TableCellStyleInfo<'table> { + cell: &'table mut TableCellFlow, + colgroup_style: Option>, + col_style: Option>, + rowgroup_style: Option<&'table Background>, + row_style: &'table Background, +} + +impl<'table> Iterator for TableCellStyleIterator<'table> { + type Item = TableCellStyleInfo<'table>; + #[inline] + fn next(&mut self) -> Option { + if let Some(ref mut row_info) = self.row_info { + if let Some(ref mut cell) = row_info.cell_iterator.next() { + // unimplemented + return None; + } else { + // next row + if let Some((group, row)) = self.row_iterator.next() { + *row_info = TableCellStyleIteratorRowInfo { + row: &row.block_flow.fragment, + rowgroup: group, + cell_iterator: row.block_flow.base.child_iter_mut() + }; + self.column_index = 0; + self.column_index_relative = 0; + self.column_index_relative_offset = 0; + // FIXME self.next() really should be up here but + // can't be without NLL, so instead it's at the + // end of the function + } else { + // out of rows + return None + } + } + } else { + // empty table + return None + } + self.next() + } +} + From 836e59e5b6b342732e2c02cab981f4ee1d7ca518 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 12:16:55 -0800 Subject: [PATCH 05/20] Handle colspan in TableCellStyleIterator --- components/layout/table.rs | 41 ++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index f92b04ea551..7faf638f295 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -945,7 +945,6 @@ struct TableCellStyleIterator<'table> { column_styles: Vec, row_iterator: TableRowAndGroupIterator<'table>, row_info: Option>, - column_index: u32, /// The index of the current column in column_styles column_index_relative: u32, /// In case of multispan columns, where we are in the @@ -974,7 +973,6 @@ impl<'table> TableCellStyleIterator<'table> { }; TableCellStyleIterator { column_styles, row_iterator, row_info, - column_index: 0, column_index_relative: 0, column_index_relative_offset: 0, } @@ -994,9 +992,41 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { #[inline] fn next(&mut self) -> Option { if let Some(ref mut row_info) = self.row_info { - if let Some(ref mut cell) = row_info.cell_iterator.next() { - // unimplemented - return None; + if let Some(cell) = row_info.cell_iterator.next() { + let rowgroup_style = row_info.rowgroup.map(|r| r.style().get_background()); + let row_style = row_info.row.style().get_background(); + let cell = cell.as_mut_table_cell(); + let (col_style, colgroup_style) = if let Some(column_style) + = self.column_styles.get(self.column_index_relative as usize) { + let styles = (column_style.col_style.clone(), column_style.colgroup_style.clone()); + // FIXME incoming_rowspan + let cell_span = cell.fragment().column_span(); + + let mut current_col = column_style; + self.column_index_relative_offset += cell_span; + while self.column_index_relative_offset >= current_col.span { + // move to the next column + self.column_index_relative += 1; + self.column_index_relative_offset -= current_col.span; + if let Some(column_style) + = self.column_styles.get(self.column_index_relative as usize) { + current_col = column_style; + } else { + break; + } + } + styles + } else { + (None, None) + }; + + return Some(TableCellStyleInfo { + cell, + colgroup_style, + col_style, + rowgroup_style, + row_style + }) } else { // next row if let Some((group, row)) = self.row_iterator.next() { @@ -1005,7 +1035,6 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { rowgroup: group, cell_iterator: row.block_flow.base.child_iter_mut() }; - self.column_index = 0; self.column_index_relative = 0; self.column_index_relative_offset = 0; // FIXME self.next() really should be up here but From f3531d1d35f4f8cee6e02a4ca01f94abad91aee1 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 12:47:31 -0800 Subject: [PATCH 06/20] Factor out BlockFlow::background_border_section() --- components/layout/block.rs | 18 ++++++++++++++++++ components/layout/display_list/builder.rs | 15 +-------------- components/layout/table.rs | 9 ++++----- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/components/layout/block.rs b/components/layout/block.rs index f87c837daf2..2eb7f76857f 100644 --- a/components/layout/block.rs +++ b/components/layout/block.rs @@ -38,6 +38,7 @@ use flow::{BaseFlow, EarlyAbsolutePositionInfo, Flow, FlowClass, ForceNonfloated use flow::{ImmutableFlowUtils, LateAbsolutePositionInfo, OpaqueFlow, FragmentationContext, FlowFlags}; use flow_list::FlowList; use fragment::{CoordinateSystem, Fragment, FragmentBorderBoxIterator, Overflow, FragmentFlags}; +use gfx::display_list::DisplayListSection; use gfx_traits::print_tree::PrintTree; use incremental::RelayoutMode; use layout_debug; @@ -1794,6 +1795,23 @@ impl BlockFlow { as_margins.to_physical(writing_mode) } + pub fn background_border_section(&self) -> DisplayListSection { + if self.base.flags.is_float() { + DisplayListSection::BackgroundAndBorders + } else if self.base + .flags + .contains(FlowFlags::IS_ABSOLUTELY_POSITIONED) + { + if self.fragment.establishes_stacking_context() { + DisplayListSection::BackgroundAndBorders + } else { + DisplayListSection::BlockBackgroundsAndBorders + } + } else { + DisplayListSection::BlockBackgroundsAndBorders + } + } + } impl Flow for BlockFlow { diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index eb5475f3d0a..a57a625097d 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -2863,20 +2863,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow { state: &mut DisplayListBuildState, border_painting_mode: BorderPaintingMode, ) { - let background_border_section = if self.base.flags.is_float() { - DisplayListSection::BackgroundAndBorders - } else if self.base - .flags - .contains(FlowFlags::IS_ABSOLUTELY_POSITIONED) - { - if self.fragment.establishes_stacking_context() { - DisplayListSection::BackgroundAndBorders - } else { - DisplayListSection::BlockBackgroundsAndBorders - } - } else { - DisplayListSection::BlockBackgroundsAndBorders - }; + let background_border_section = self.background_border_section(); state.processing_scrolling_overflow_element = self.has_scrolling_overflow(); diff --git a/components/layout/table.rs b/components/layout/table.rs index 7faf638f295..daea0bd600e 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -996,8 +996,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { let rowgroup_style = row_info.rowgroup.map(|r| r.style().get_background()); let row_style = row_info.row.style().get_background(); let cell = cell.as_mut_table_cell(); - let (col_style, colgroup_style) = if let Some(column_style) - = self.column_styles.get(self.column_index_relative as usize) { + let (col_style, colgroup_style) = if let Some(column_style) = + self.column_styles.get(self.column_index_relative as usize) { let styles = (column_style.col_style.clone(), column_style.colgroup_style.clone()); // FIXME incoming_rowspan let cell_span = cell.fragment().column_span(); @@ -1008,8 +1008,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { // move to the next column self.column_index_relative += 1; self.column_index_relative_offset -= current_col.span; - if let Some(column_style) - = self.column_styles.get(self.column_index_relative as usize) { + if let Some(column_style) = + self.column_styles.get(self.column_index_relative as usize) { current_col = column_style; } else { break; @@ -1052,4 +1052,3 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { self.next() } } - From f7ac5d712f6581c20f32769c9992d8d7667fc7f6 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 13:05:02 -0800 Subject: [PATCH 07/20] Add build_display_list_for_background_if_applicable_with_background --- components/layout/display_list/builder.rs | 31 +++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index a57a625097d..364991d2061 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -531,6 +531,18 @@ pub trait FragmentDisplayListBuilding { absolute_bounds: &Rect, ); + /// Same as build_display_list_for_background_if_applicable, but lets you + /// override the actual background used + fn build_display_list_for_background_if_applicable_with_background( + &self, + state: &mut DisplayListBuildState, + style: &ComputedValues, + background: &style_structs::Background, + background_color: RGBA, + display_list_section: DisplayListSection, + absolute_bounds: &Rect, + ); + /// Determines where to place an element background image or gradient. /// /// Photos have their resolution as intrinsic size while gradients have @@ -929,13 +941,28 @@ impl FragmentDisplayListBuilding for Fragment { style: &ComputedValues, display_list_section: DisplayListSection, absolute_bounds: &Rect, + ) { + let background = style.get_background(); + let background_color = style.resolve_color(background.background_color); + // XXXManishearth the below method should ideally use an iterator over + // backgrounds + self.build_display_list_for_background_if_applicable_with_background( + state, style, background, background_color, display_list_section, absolute_bounds) + } + + fn build_display_list_for_background_if_applicable_with_background( + &self, + state: &mut DisplayListBuildState, + style: &ComputedValues, + background: &style_structs::Background, + background_color: RGBA, + display_list_section: DisplayListSection, + absolute_bounds: &Rect, ) { // FIXME: This causes a lot of background colors to be displayed when they are clearly not // needed. We could use display list optimization to clean this up, but it still seems // inefficient. What we really want is something like "nearest ancestor element that // doesn't have a fragment". - let background = style.get_background(); - let background_color = style.resolve_color(background.background_color); // 'background-clip' determines the area within which the background is painted. // http://dev.w3.org/csswg/css-backgrounds-3/#the-background-clip From 62328466990e3ea436e5d05486eb26bc4ce0d799 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 13:30:48 -0800 Subject: [PATCH 08/20] Add stacking_relative_border_box_for_display_list --- components/layout/display_list/builder.rs | 61 ++++++----------------- components/layout/flow.rs | 13 +++++ 2 files changed, 27 insertions(+), 47 deletions(-) diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index 364991d2061..c1bbfe70a72 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -60,7 +60,7 @@ use style::computed_values::overflow_x::T as StyleOverflow; use style::computed_values::pointer_events::T as PointerEvents; use style::computed_values::position::T as StylePosition; use style::computed_values::visibility::T as Visibility; -use style::logical_geometry::{LogicalMargin, LogicalPoint, LogicalRect, LogicalSize, WritingMode}; +use style::logical_geometry::{LogicalMargin, LogicalPoint, LogicalRect}; use style::properties::ComputedValues; use style::properties::style_structs; use style::servo::restyle_damage::ServoRestyleDamage; @@ -651,17 +651,11 @@ pub trait FragmentDisplayListBuilding { /// * `state`: The display building state, including the display list currently /// under construction and other metadata useful for constructing it. /// * `dirty`: The dirty rectangle in the coordinate system of the owning flow. - /// * `stacking_relative_flow_origin`: Position of the origin of the owning flow with respect - /// to its nearest ancestor stacking context. - /// * `relative_containing_block_size`: The size of the containing block that - /// `position: relative` makes use of. /// * `clip`: The region to clip the display items to. fn build_display_list( &mut self, state: &mut DisplayListBuildState, - stacking_relative_flow_origin: &Vector2D, - relative_containing_block_size: &LogicalSize, - relative_containing_block_mode: WritingMode, + stacking_relative_border_box: Rect, border_painting_mode: BorderPaintingMode, display_list_section: DisplayListSection, clip: &Rect, @@ -1721,9 +1715,7 @@ impl FragmentDisplayListBuilding for Fragment { fn build_display_list( &mut self, state: &mut DisplayListBuildState, - stacking_relative_flow_origin: &Vector2D, - relative_containing_block_size: &LogicalSize, - relative_containing_block_mode: WritingMode, + stacking_relative_border_box: Rect, border_painting_mode: BorderPaintingMode, display_list_section: DisplayListSection, clip: &Rect, @@ -1733,19 +1725,9 @@ impl FragmentDisplayListBuilding for Fragment { return; } - // Compute the fragment position relative to the parent stacking context. If the fragment - // itself establishes a stacking context, then the origin of its position will be (0, 0) - // for the purposes of this computation. - let stacking_relative_border_box = self.stacking_relative_border_box( - stacking_relative_flow_origin, - relative_containing_block_size, - relative_containing_block_mode, - CoordinateSystem::Own, - ); - debug!( - "Fragment::build_display_list at rel={:?}, abs={:?}, flow origin={:?}: {:?}", - self.border_box, stacking_relative_border_box, stacking_relative_flow_origin, self + "Fragment::build_display_list at rel={:?}, abs={:?}: {:?}", + self.border_box, stacking_relative_border_box, self ); // Check the clip rect. If there's nothing to render at all, don't even construct display @@ -2893,17 +2875,12 @@ impl BlockFlowDisplayListBuilding for BlockFlow { let background_border_section = self.background_border_section(); state.processing_scrolling_overflow_element = self.has_scrolling_overflow(); - + let stacking_relative_border_box = + self.base.stacking_relative_border_box_for_display_list(&self.fragment); // Add the box that starts the block context. self.fragment.build_display_list( state, - &self.base.stacking_relative_position, - &self.base - .early_absolute_position_info - .relative_containing_block_size, - self.base - .early_absolute_position_info - .relative_containing_block_mode, + stacking_relative_border_box, border_painting_mode, background_border_section, &self.base.clip, @@ -3006,15 +2983,11 @@ impl InlineFlowDisplayListBuilding for InlineFlow { index: usize, ) { let fragment = self.fragments.fragments.get_mut(index).unwrap(); + let stacking_relative_border_box = + self.base.stacking_relative_border_box_for_display_list(fragment); fragment.build_display_list( state, - &self.base.stacking_relative_position, - &self.base - .early_absolute_position_info - .relative_containing_block_size, - self.base - .early_absolute_position_info - .relative_containing_block_mode, + stacking_relative_border_box, BorderPaintingMode::Separate, DisplayListSection::Content, &self.base.clip, @@ -3066,17 +3039,11 @@ impl ListItemFlowDisplayListBuilding for ListItemFlow { fn build_display_list_for_list_item(&mut self, state: &mut DisplayListBuildState) { // Draw the marker, if applicable. for marker in &mut self.marker_fragments { + let stacking_relative_border_box = + self.block_flow.base.stacking_relative_border_box_for_display_list(marker); marker.build_display_list( state, - &self.block_flow.base.stacking_relative_position, - &self.block_flow - .base - .early_absolute_position_info - .relative_containing_block_size, - self.block_flow - .base - .early_absolute_position_info - .relative_containing_block_mode, + stacking_relative_border_box, BorderPaintingMode::Separate, DisplayListSection::Content, &self.block_flow.base.clip, diff --git a/components/layout/flow.rs b/components/layout/flow.rs index 30877defa43..399a21087c4 100644 --- a/components/layout/flow.rs +++ b/components/layout/flow.rs @@ -1155,6 +1155,19 @@ impl BaseFlow { self.speculated_float_placement_out.left > Au(0) || self.speculated_float_placement_out.right > Au(0) } + + + /// Compute the fragment position relative to the parent stacking context. If the fragment + /// itself establishes a stacking context, then the origin of its position will be (0, 0) + /// for the purposes of this computation. + pub fn stacking_relative_border_box_for_display_list(&self, fragment: &Fragment) -> Rect { + fragment.stacking_relative_border_box( + &self.stacking_relative_position, + &self.early_absolute_position_info.relative_containing_block_size, + self.early_absolute_position_info.relative_containing_block_mode, + CoordinateSystem::Own, + ) + } } impl<'a> ImmutableFlowUtils for &'a Flow { From db6ec58e6b971aa040739f0dc77701c02a889e1c Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 13:57:59 -0800 Subject: [PATCH 09/20] Generate display lists for table cells during display list generation for their table parent --- components/layout/block.rs | 1 - components/layout/display_list/builder.rs | 22 +++++++++ components/layout/table.rs | 56 +++++++++++++++++++++-- components/layout/table_cell.rs | 24 ++-------- components/layout/table_row.rs | 18 ++------ 5 files changed, 83 insertions(+), 38 deletions(-) diff --git a/components/layout/block.rs b/components/layout/block.rs index 2eb7f76857f..7b809ce879e 100644 --- a/components/layout/block.rs +++ b/components/layout/block.rs @@ -1811,7 +1811,6 @@ impl BlockFlow { DisplayListSection::BlockBackgroundsAndBorders } } - } impl Flow for BlockFlow { diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index c1bbfe70a72..d391fbcbb1a 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -2330,6 +2330,12 @@ pub trait BlockFlowDisplayListBuilding { border_painting_mode: BorderPaintingMode, ); + fn build_display_list_for_background_if_applicable_with_background( + &self, + state: &mut DisplayListBuildState, + background: &style_structs::Background, + background_color: RGBA); + fn block_stacking_context_type( &self, flags: StackingContextCollectionFlags, @@ -2892,6 +2898,22 @@ impl BlockFlowDisplayListBuilding for BlockFlow { state.processing_scrolling_overflow_element = false; } + fn build_display_list_for_background_if_applicable_with_background( + &self, + state: &mut DisplayListBuildState, + background: &style_structs::Background, + background_color: RGBA) { + let stacking_relative_border_box = + self.base.stacking_relative_border_box_for_display_list(&self.fragment); + let background_border_section = self.background_border_section(); + + self.fragment.build_display_list_for_background_if_applicable_with_background( + state, self.fragment.style(), background, background_color, + background_border_section, &stacking_relative_border_box + ) + + } + #[inline] fn block_stacking_context_type( &self, diff --git a/components/layout/table.rs b/components/layout/table.rs index daea0bd600e..b888df65fe5 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -531,6 +531,13 @@ impl Flow for TableFlow { }; self.block_flow.build_display_list_for_block(state, border_painting_mode); + + let column_styles = self.column_styles(); + let iter = TableCellStyleIterator::new(&mut self.block_flow.base, column_styles); + let cv = self.block_flow.fragment.style(); + for mut style in iter { + style.build_display_list(state, cv) + } } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { @@ -959,9 +966,8 @@ struct TableCellStyleIteratorRowInfo<'table> { } impl<'table> TableCellStyleIterator<'table> { - fn new(table: &'table mut TableFlow) -> Self { - let column_styles = table.column_styles(); - let mut row_iterator = TableRowAndGroupIterator::new(&mut table.block_flow.base); + fn new(base: &'table mut BaseFlow, column_styles: Vec) -> Self { + let mut row_iterator = TableRowAndGroupIterator::new(base); let row_info = if let Some((group, row)) = row_iterator.next() { Some(TableCellStyleIteratorRowInfo { row: &row.block_flow.fragment, @@ -1000,7 +1006,7 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { self.column_styles.get(self.column_index_relative as usize) { let styles = (column_style.col_style.clone(), column_style.colgroup_style.clone()); // FIXME incoming_rowspan - let cell_span = cell.fragment().column_span(); + let cell_span = cell.column_span; let mut current_col = column_style; self.column_index_relative_offset += cell_span; @@ -1052,3 +1058,45 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { self.next() } } + +impl<'table> TableCellStyleInfo<'table> { + fn build_display_list(&mut self, mut state: &mut DisplayListBuildState, table_style: &'table ComputedValues) { + if !self.cell.visible { + return + } + let border_painting_mode = match self.cell.block_flow + .fragment + .style + .get_inheritedtable() + .border_collapse { + border_collapse::T::Separate => BorderPaintingMode::Separate, + border_collapse::T::Collapse => BorderPaintingMode::Collapse(&self.cell.collapsed_borders), + }; + { + let cell_flow = &self.cell.block_flow; + // XXXManishearth the color should be resolved relative to the style itself + // which we don't have here + let build_dl = |bg, state: &mut &mut DisplayListBuildState| { + cell_flow.build_display_list_for_background_if_applicable_with_background( + state, bg, table_style.resolve_color(bg.background_color) + ) + }; + + + build_dl(table_style.get_background(), &mut state); + + if let Some(ref bg) = self.colgroup_style { + build_dl(&bg, &mut state); + } + if let Some(ref bg) = self.col_style { + build_dl(&bg, &mut state); + } + if let Some(ref bg) = self.rowgroup_style { + build_dl(bg, &mut state); + } + build_dl(self.row_style, &mut state); + } + + self.cell.block_flow.build_display_list_for_block(state, border_painting_mode) + } +} diff --git a/components/layout/table_cell.rs b/components/layout/table_cell.rs index 438e30d96fe..9fe04eb149f 100644 --- a/components/layout/table_cell.rs +++ b/components/layout/table_cell.rs @@ -9,9 +9,8 @@ use app_units::Au; use block::{BlockFlow, ISizeAndMarginsComputer, MarginsMayCollapseFlag}; use context::LayoutContext; -use display_list::{BlockFlowDisplayListBuilding, BorderPaintingMode}; -use display_list::{DisplayListBuildState, StackingContextCollectionFlags}; -use display_list::StackingContextCollectionState; +use display_list::{BlockFlowDisplayListBuilding, DisplayListBuildState}; +use display_list::{StackingContextCollectionFlags, StackingContextCollectionState}; use euclid::{Point2D, Rect, SideOffsets2D, Size2D}; use flow::{Flow, FlowClass, FlowFlags, GetBaseFlow, OpaqueFlow}; use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; @@ -20,7 +19,6 @@ use layout_debug; use model::MaybeAuto; use script_layout_interface::wrapper_traits::ThreadSafeLayoutNode; use std::fmt; -use style::computed_values::border_collapse::T as BorderCollapse; use style::logical_geometry::{LogicalMargin, LogicalRect, LogicalSize, WritingMode}; use style::properties::ComputedValues; use style::values::computed::Color; @@ -248,21 +246,9 @@ impl Flow for TableCellFlow { self.block_flow.update_late_computed_block_position_if_necessary(block_position) } - fn build_display_list(&mut self, state: &mut DisplayListBuildState) { - if !self.visible { - return - } - - let border_painting_mode = match self.block_flow - .fragment - .style - .get_inheritedtable() - .border_collapse { - BorderCollapse::Separate => BorderPaintingMode::Separate, - BorderCollapse::Collapse => BorderPaintingMode::Collapse(&self.collapsed_borders), - }; - - self.block_flow.build_display_list_for_block(state, border_painting_mode) + fn build_display_list(&mut self, _: &mut DisplayListBuildState) { + // This is handled by TableCellStyleInfo::build_display_list() + // when the containing table builds its display list } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { diff --git a/components/layout/table_row.rs b/components/layout/table_row.rs index a1c91417cd7..be83f6321e3 100644 --- a/components/layout/table_row.rs +++ b/components/layout/table_row.rs @@ -9,9 +9,8 @@ use app_units::Au; use block::{BlockFlow, ISizeAndMarginsComputer}; use context::LayoutContext; -use display_list::{BlockFlowDisplayListBuilding, BorderPaintingMode}; -use display_list::{DisplayListBuildState, StackingContextCollectionFlags}; -use display_list::StackingContextCollectionState; +use display_list::{BlockFlowDisplayListBuilding, DisplayListBuildState}; +use display_list::{StackingContextCollectionFlags, StackingContextCollectionState}; use euclid::Point2D; use flow::{EarlyAbsolutePositionInfo, Flow, FlowClass, ImmutableFlowUtils, GetBaseFlow, OpaqueFlow}; use flow_list::MutFlowListIterator; @@ -467,17 +466,8 @@ impl Flow for TableRowFlow { self.block_flow.update_late_computed_block_position_if_necessary(block_position) } - fn build_display_list(&mut self, state: &mut DisplayListBuildState) { - let border_painting_mode = match self.block_flow - .fragment - .style - .get_inheritedtable() - .border_collapse { - BorderCollapse::Separate => BorderPaintingMode::Separate, - BorderCollapse::Collapse => BorderPaintingMode::Hidden, - }; - - self.block_flow.build_display_list_for_block(state, border_painting_mode); + fn build_display_list(&mut self, _: &mut DisplayListBuildState) { + // handled in TableCellStyleInfo::build_display_list } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { From 5cd15eeb54bf4e2f53059a8ba2245d919f36cafe Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 14:12:22 -0800 Subject: [PATCH 10/20] Don't redraw backgrounds that we've already drawn --- components/layout/table.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index b888df65fe5..8c2d4e86c96 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -20,8 +20,7 @@ use gfx_traits::print_tree::PrintTree; use layout_debug; use model::{IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto}; use servo_arc::Arc; -use std::cmp; -use std::fmt; +use std::{cmp, fmt, ptr}; use style::computed_values::{border_collapse, border_spacing, table_layout}; use style::context::SharedStyleContext; use style::logical_geometry::LogicalSize; @@ -1074,12 +1073,20 @@ impl<'table> TableCellStyleInfo<'table> { }; { let cell_flow = &self.cell.block_flow; + let mut bg_ptr = ptr::null(); + // XXXManishearth the color should be resolved relative to the style itself // which we don't have here - let build_dl = |bg, state: &mut &mut DisplayListBuildState| { + let mut build_dl = |bg, state: &mut &mut DisplayListBuildState| { + // Don't redraw backgrounds that we've already drawn + if bg_ptr == bg as *const _ { + return; + } cell_flow.build_display_list_for_background_if_applicable_with_background( state, bg, table_style.resolve_color(bg.background_color) - ) + ); + + bg_ptr = bg as *const _; }; From 265a2ab2ccca7594a1d7bed18e7f50c7a034cb84 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 15:48:00 -0800 Subject: [PATCH 11/20] Allow for build_display_list to be called immutably --- components/layout/block.rs | 2 +- components/layout/display_list/builder.rs | 51 +++++++++++++++++++---- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/components/layout/block.rs b/components/layout/block.rs index 7b809ce879e..d7505e7b8ba 100644 --- a/components/layout/block.rs +++ b/components/layout/block.rs @@ -1777,7 +1777,7 @@ impl BlockFlow { } } - pub fn has_scrolling_overflow(&mut self) -> bool { + pub fn has_scrolling_overflow(&self) -> bool { self.flags.contains(BlockFlowFlags::HAS_SCROLLING_OVERFLOW) } diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index d391fbcbb1a..d57c663ecd5 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -661,6 +661,18 @@ pub trait FragmentDisplayListBuilding { clip: &Rect, ); + /// build_display_list, but don't update the restyle damage + /// + /// Must be paired with a self.restyle_damage.remove(REPAINT) somewhere + fn build_display_list_no_damage( + &self, + state: &mut DisplayListBuildState, + stacking_relative_border_box: Rect, + border_painting_mode: BorderPaintingMode, + display_list_section: DisplayListSection, + clip: &Rect, + ); + /// Builds the display items necessary to paint the selection and/or caret for this fragment, /// if any. fn build_display_items_for_selection_if_necessary( @@ -695,7 +707,7 @@ pub trait FragmentDisplayListBuilding { /// A helper method that `build_display_list` calls to create per-fragment-type display items. fn build_fragment_type_specific_display_items( - &mut self, + &self, state: &mut DisplayListBuildState, stacking_relative_border_box: &Rect, clip: &Rect, @@ -1721,6 +1733,18 @@ impl FragmentDisplayListBuilding for Fragment { clip: &Rect, ) { self.restyle_damage.remove(ServoRestyleDamage::REPAINT); + self.build_display_list_no_damage(state, stacking_relative_border_box, + border_painting_mode, display_list_section, clip) + } + + fn build_display_list_no_damage( + &self, + state: &mut DisplayListBuildState, + stacking_relative_border_box: Rect, + border_painting_mode: BorderPaintingMode, + display_list_section: DisplayListSection, + clip: &Rect, + ) { if self.style().get_inheritedbox().visibility != Visibility::Visible { return; } @@ -1839,7 +1863,7 @@ impl FragmentDisplayListBuilding for Fragment { } fn build_fragment_type_specific_display_items( - &mut self, + &self, state: &mut DisplayListBuildState, stacking_relative_border_box: &Rect, clip: &Rect, @@ -1963,7 +1987,7 @@ impl FragmentDisplayListBuilding for Fragment { state.add_display_item(item); } }, - SpecificFragmentInfo::Image(ref mut image_fragment) => { + SpecificFragmentInfo::Image(ref image_fragment) => { // Place the image into the display list. if let Some(ref image) = image_fragment.image { let base = state.create_base_display_item( @@ -2329,7 +2353,11 @@ pub trait BlockFlowDisplayListBuilding { state: &mut DisplayListBuildState, border_painting_mode: BorderPaintingMode, ); - + fn build_display_list_for_block_no_damage( + &self, + state: &mut DisplayListBuildState, + border_painting_mode: BorderPaintingMode, + ); fn build_display_list_for_background_if_applicable_with_background( &self, state: &mut DisplayListBuildState, @@ -2873,8 +2901,8 @@ impl BlockFlowDisplayListBuilding for BlockFlow { self.base.collect_stacking_contexts_for_children(state); } - fn build_display_list_for_block( - &mut self, + fn build_display_list_for_block_no_damage( + &self, state: &mut DisplayListBuildState, border_painting_mode: BorderPaintingMode, ) { @@ -2884,7 +2912,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow { let stacking_relative_border_box = self.base.stacking_relative_border_box_for_display_list(&self.fragment); // Add the box that starts the block context. - self.fragment.build_display_list( + self.fragment.build_display_list_no_damage( state, stacking_relative_border_box, border_painting_mode, @@ -2898,6 +2926,15 @@ impl BlockFlowDisplayListBuilding for BlockFlow { state.processing_scrolling_overflow_element = false; } + fn build_display_list_for_block( + &mut self, + state: &mut DisplayListBuildState, + border_painting_mode: BorderPaintingMode, + ) { + self.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); + self.build_display_list_for_block_no_damage(state, border_painting_mode); + } + fn build_display_list_for_background_if_applicable_with_background( &self, state: &mut DisplayListBuildState, From 21140e7a0ace4c885b8ca0107dabf290ebca4209 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 16:00:42 -0800 Subject: [PATCH 12/20] Make TableCellStyleIterator operate on immutable flows --- components/layout/table.rs | 76 ++++++++++++++++++++++++++------- components/layout/table_cell.rs | 7 +++ 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 8c2d4e86c96..57185ee45da 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -14,7 +14,7 @@ use display_list::{BlockFlowDisplayListBuilding, BorderPaintingMode}; use display_list::{DisplayListBuildState, StackingContextCollectionFlags, StackingContextCollectionState}; use euclid::Point2D; use flow::{BaseFlow, EarlyAbsolutePositionInfo, Flow, FlowClass, ImmutableFlowUtils, GetBaseFlow, OpaqueFlow}; -use flow_list::MutFlowListIterator; +use flow_list::{FlowListIterator, MutFlowListIterator}; use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use gfx_traits::print_tree::PrintTree; use layout_debug; @@ -882,20 +882,66 @@ enum NextBlockCollapsedBorders<'a> { /// Iterator over all the rows of a table, which also /// provides the Fragment for rowgroups if any struct TableRowAndGroupIterator<'a> { - kids: MutFlowListIterator<'a>, - group: Option<(&'a Fragment, MutFlowListIterator<'a>)> + kids: FlowListIterator<'a>, + group: Option<(&'a Fragment, FlowListIterator<'a>)> } impl<'a> TableRowAndGroupIterator<'a> { - fn new(base: &'a mut BaseFlow) -> Self { + fn new(base: &'a BaseFlow) -> Self { TableRowAndGroupIterator { - kids: base.child_iter_mut(), + kids: base.child_iter(), group: None, } } } impl<'a> Iterator for TableRowAndGroupIterator<'a> { + type Item = (Option<&'a Fragment>, &'a TableRowFlow); + #[inline] + fn next(&mut self) -> Option { + // If we're inside a rowgroup, iterate through the rowgroup's children. + if let Some(ref mut group) = self.group { + if let Some(grandkid) = group.1.next() { + return Some((Some(group.0), grandkid.as_table_row())) + } + } + // Otherwise, iterate through the table's children. + self.group = None; + match self.kids.next() { + Some(kid) => { + if kid.is_table_rowgroup() { + let mut rowgroup = kid.as_table_rowgroup(); + let iter = rowgroup.block_flow.base.child_iter(); + self.group = Some((&rowgroup.block_flow.fragment, iter)); + self.next() + } else if kid.is_table_row() { + Some((None, kid.as_table_row())) + } else { + self.next() // Skip children that are not rows or rowgroups + } + } + None => None + } + } +} + +/// Iterator over all the rows of a table, which also +/// provides the Fragment for rowgroups if any +struct MutTableRowAndGroupIterator<'a> { + kids: MutFlowListIterator<'a>, + group: Option<(&'a Fragment, MutFlowListIterator<'a>)> +} + +impl<'a> MutTableRowAndGroupIterator<'a> { + fn new(base: &'a mut BaseFlow) -> Self { + MutTableRowAndGroupIterator { + kids: base.child_iter_mut(), + group: None, + } + } +} + +impl<'a> Iterator for MutTableRowAndGroupIterator<'a> { type Item = (Option<&'a Fragment>, &'a mut TableRowFlow); #[inline] fn next(&mut self) -> Option { @@ -926,11 +972,11 @@ impl<'a> Iterator for TableRowAndGroupIterator<'a> { } /// Iterator over all the rows of a table -struct TableRowIterator<'a>(TableRowAndGroupIterator<'a>); +struct TableRowIterator<'a>(MutTableRowAndGroupIterator<'a>); impl<'a> TableRowIterator<'a> { fn new(base: &'a mut BaseFlow) -> Self { - TableRowIterator(TableRowAndGroupIterator::new(base)) + TableRowIterator(MutTableRowAndGroupIterator::new(base)) } } @@ -961,7 +1007,7 @@ struct TableCellStyleIterator<'table> { struct TableCellStyleIteratorRowInfo<'table> { row: &'table Fragment, rowgroup: Option<&'table Fragment>, - cell_iterator: MutFlowListIterator<'table>, + cell_iterator: FlowListIterator<'table>, } impl<'table> TableCellStyleIterator<'table> { @@ -971,7 +1017,7 @@ impl<'table> TableCellStyleIterator<'table> { Some(TableCellStyleIteratorRowInfo { row: &row.block_flow.fragment, rowgroup: group, - cell_iterator: row.block_flow.base.child_iter_mut() + cell_iterator: row.block_flow.base.child_iter() }) } else { None @@ -985,7 +1031,7 @@ impl<'table> TableCellStyleIterator<'table> { } struct TableCellStyleInfo<'table> { - cell: &'table mut TableCellFlow, + cell: &'table TableCellFlow, colgroup_style: Option>, col_style: Option>, rowgroup_style: Option<&'table Background>, @@ -1000,7 +1046,7 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { if let Some(cell) = row_info.cell_iterator.next() { let rowgroup_style = row_info.rowgroup.map(|r| r.style().get_background()); let row_style = row_info.row.style().get_background(); - let cell = cell.as_mut_table_cell(); + let cell = cell.as_table_cell(); let (col_style, colgroup_style) = if let Some(column_style) = self.column_styles.get(self.column_index_relative as usize) { let styles = (column_style.col_style.clone(), column_style.colgroup_style.clone()); @@ -1038,7 +1084,7 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { *row_info = TableCellStyleIteratorRowInfo { row: &row.block_flow.fragment, rowgroup: group, - cell_iterator: row.block_flow.base.child_iter_mut() + cell_iterator: row.block_flow.base.child_iter() }; self.column_index_relative = 0; self.column_index_relative_offset = 0; @@ -1059,7 +1105,7 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { } impl<'table> TableCellStyleInfo<'table> { - fn build_display_list(&mut self, mut state: &mut DisplayListBuildState, table_style: &'table ComputedValues) { + fn build_display_list(&self, mut state: &mut DisplayListBuildState, table_style: &'table ComputedValues) { if !self.cell.visible { return } @@ -1103,7 +1149,7 @@ impl<'table> TableCellStyleInfo<'table> { } build_dl(self.row_style, &mut state); } - - self.cell.block_flow.build_display_list_for_block(state, border_painting_mode) + // the restyle damage will be set in TableCellFlow::build_display_list() + self.cell.block_flow.build_display_list_for_block_no_damage(state, border_painting_mode) } } diff --git a/components/layout/table_cell.rs b/components/layout/table_cell.rs index 9fe04eb149f..65e613dfbd2 100644 --- a/components/layout/table_cell.rs +++ b/components/layout/table_cell.rs @@ -247,8 +247,15 @@ impl Flow for TableCellFlow { } fn build_display_list(&mut self, _: &mut DisplayListBuildState) { + use style::servo::restyle_damage::ServoRestyleDamage; // This is handled by TableCellStyleInfo::build_display_list() // when the containing table builds its display list + + if self.visible { + // we skip setting the damage in TableCellStyleInfo::build_display_list() + // because we only have immutable access + self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); + } } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { From b72a50d50a77e00ce7fcd6f035160cf71c3b6307 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 16:09:04 -0800 Subject: [PATCH 13/20] Pass down full ComputedValues to TableCellStyleInfo; use for correct color computation --- components/layout/table.rs | 75 +++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 57185ee45da..3e102c099b0 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -19,13 +19,11 @@ use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use gfx_traits::print_tree::PrintTree; use layout_debug; use model::{IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto}; -use servo_arc::Arc; use std::{cmp, fmt, ptr}; use style::computed_values::{border_collapse, border_spacing, table_layout}; use style::context::SharedStyleContext; use style::logical_geometry::LogicalSize; use style::properties::ComputedValues; -use style::properties::style_structs::Background; use style::servo::restyle_damage::ServoRestyleDamage; use style::values::CSSFloat; use style::values::computed::LengthOrPercentageOrAuto; @@ -214,7 +212,7 @@ impl TableFlow { // so that we can filter_map let group = group.as_table_colgroup(); let colgroup_style = group.fragment.as_ref() - .map(|f| f.style().clone_background()); + .map(|f| f.style()); // The colgroup's span attribute is only relevant when // it has no children @@ -228,8 +226,8 @@ impl TableFlow { // XXXManishearth Arc-cloning colgroup_style is suboptimal styles.push(ColumnStyle { span: col.column_span(), - colgroup_style: colgroup_style.clone(), - col_style: Some(col.style().clone_background()), + colgroup_style: colgroup_style, + col_style: Some(col.style()), }) } } @@ -531,11 +529,9 @@ impl Flow for TableFlow { self.block_flow.build_display_list_for_block(state, border_painting_mode); - let column_styles = self.column_styles(); - let iter = TableCellStyleIterator::new(&mut self.block_flow.base, column_styles); - let cv = self.block_flow.fragment.style(); + let iter = TableCellStyleIterator::new(&self); for mut style in iter { - style.build_display_list(state, cv) + style.build_display_list(state) } } @@ -577,10 +573,10 @@ impl Flow for TableFlow { // for rows (and have no borrowing issues between // holding on to both ColumnStyle<'table> and // the rows) -struct ColumnStyle { +struct ColumnStyle<'table> { span: u32, - colgroup_style: Option>, - col_style: Option>, + colgroup_style: Option<&'table ComputedValues>, + col_style: Option<&'table ComputedValues>, } impl fmt::Debug for TableFlow { @@ -994,9 +990,10 @@ impl<'a> Iterator for TableRowIterator<'a> { /// Used for correctly handling table layers from /// https://drafts.csswg.org/css2/tables.html#table-layers struct TableCellStyleIterator<'table> { - column_styles: Vec, + column_styles: Vec>, row_iterator: TableRowAndGroupIterator<'table>, row_info: Option>, + table_style: &'table ComputedValues, /// The index of the current column in column_styles column_index_relative: u32, /// In case of multispan columns, where we are in the @@ -1011,8 +1008,9 @@ struct TableCellStyleIteratorRowInfo<'table> { } impl<'table> TableCellStyleIterator<'table> { - fn new(base: &'table mut BaseFlow, column_styles: Vec) -> Self { - let mut row_iterator = TableRowAndGroupIterator::new(base); + fn new(table: &'table TableFlow) -> Self { + let column_styles = table.column_styles(); + let mut row_iterator = TableRowAndGroupIterator::new(&table.block_flow.base); let row_info = if let Some((group, row)) = row_iterator.next() { Some(TableCellStyleIteratorRowInfo { row: &row.block_flow.fragment, @@ -1026,16 +1024,18 @@ impl<'table> TableCellStyleIterator<'table> { column_styles, row_iterator, row_info, column_index_relative: 0, column_index_relative_offset: 0, + table_style: table.block_flow.fragment.style(), } } } struct TableCellStyleInfo<'table> { cell: &'table TableCellFlow, - colgroup_style: Option>, - col_style: Option>, - rowgroup_style: Option<&'table Background>, - row_style: &'table Background, + table_style: &'table ComputedValues, + colgroup_style: Option<&'table ComputedValues>, + col_style: Option<&'table ComputedValues>, + rowgroup_style: Option<&'table ComputedValues>, + row_style: &'table ComputedValues, } impl<'table> Iterator for TableCellStyleIterator<'table> { @@ -1044,8 +1044,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { fn next(&mut self) -> Option { if let Some(ref mut row_info) = self.row_info { if let Some(cell) = row_info.cell_iterator.next() { - let rowgroup_style = row_info.rowgroup.map(|r| r.style().get_background()); - let row_style = row_info.row.style().get_background(); + let rowgroup_style = row_info.rowgroup.map(|r| r.style()); + let row_style = row_info.row.style(); let cell = cell.as_table_cell(); let (col_style, colgroup_style) = if let Some(column_style) = self.column_styles.get(self.column_index_relative as usize) { @@ -1076,7 +1076,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { colgroup_style, col_style, rowgroup_style, - row_style + row_style, + table_style: self.table_style, }) } else { // next row @@ -1105,7 +1106,7 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { } impl<'table> TableCellStyleInfo<'table> { - fn build_display_list(&self, mut state: &mut DisplayListBuildState, table_style: &'table ComputedValues) { + fn build_display_list(&self, mut state: &mut DisplayListBuildState) { if !self.cell.visible { return } @@ -1119,33 +1120,33 @@ impl<'table> TableCellStyleInfo<'table> { }; { let cell_flow = &self.cell.block_flow; - let mut bg_ptr = ptr::null(); + let mut sty_ptr = ptr::null(); - // XXXManishearth the color should be resolved relative to the style itself - // which we don't have here - let mut build_dl = |bg, state: &mut &mut DisplayListBuildState| { + let mut build_dl = |sty: &ComputedValues, state: &mut &mut DisplayListBuildState| { // Don't redraw backgrounds that we've already drawn - if bg_ptr == bg as *const _ { + if sty_ptr == sty as *const _ { return; } + let background = sty.get_background(); + let background_color = sty.resolve_color(background.background_color); cell_flow.build_display_list_for_background_if_applicable_with_background( - state, bg, table_style.resolve_color(bg.background_color) + state, background, background_color ); - bg_ptr = bg as *const _; + sty_ptr = sty as *const _; }; - build_dl(table_style.get_background(), &mut state); + build_dl(self.table_style, &mut state); - if let Some(ref bg) = self.colgroup_style { - build_dl(&bg, &mut state); + if let Some(ref sty) = self.colgroup_style { + build_dl(&sty, &mut state); } - if let Some(ref bg) = self.col_style { - build_dl(&bg, &mut state); + if let Some(ref sty) = self.col_style { + build_dl(&sty, &mut state); } - if let Some(ref bg) = self.rowgroup_style { - build_dl(bg, &mut state); + if let Some(ref sty) = self.rowgroup_style { + build_dl(sty, &mut state); } build_dl(self.row_style, &mut state); } From cfa81e8b79b86d8699eed1cf52f1927d83baf967 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 17:17:22 -0800 Subject: [PATCH 14/20] Refactor column index advancing into its own method --- components/layout/table.rs | 108 +++++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 39 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 3e102c099b0..36aa1d10cb8 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -994,15 +994,12 @@ struct TableCellStyleIterator<'table> { row_iterator: TableRowAndGroupIterator<'table>, row_info: Option>, table_style: &'table ComputedValues, - /// The index of the current column in column_styles - column_index_relative: u32, - /// In case of multispan columns, where we are in the - /// span of the current element - column_index_relative_offset: u32, + column_index: TableCellColumnIndexData, + } struct TableCellStyleIteratorRowInfo<'table> { - row: &'table Fragment, + row: &'table TableRowFlow, rowgroup: Option<&'table Fragment>, cell_iterator: FlowListIterator<'table>, } @@ -1013,7 +1010,7 @@ impl<'table> TableCellStyleIterator<'table> { let mut row_iterator = TableRowAndGroupIterator::new(&table.block_flow.base); let row_info = if let Some((group, row)) = row_iterator.next() { Some(TableCellStyleIteratorRowInfo { - row: &row.block_flow.fragment, + row: &row, rowgroup: group, cell_iterator: row.block_flow.base.child_iter() }) @@ -1022,8 +1019,7 @@ impl<'table> TableCellStyleIterator<'table> { }; TableCellStyleIterator { column_styles, row_iterator, row_info, - column_index_relative: 0, - column_index_relative_offset: 0, + column_index: Default::default(), table_style: table.block_flow.fragment.style(), } } @@ -1038,39 +1034,76 @@ struct TableCellStyleInfo<'table> { row_style: &'table ComputedValues, } +struct TableCellColumnIndexData { + /// Which column this is in the table + pub absolute: u32, + /// The index of the current column in column_styles + /// (i.e. which element it is) + pub relative: u32, + /// In case of multispan s, where we are in the + /// span of the current element + pub relative_offset: u32, +} + +impl Default for TableCellColumnIndexData { + fn default() -> Self { + TableCellColumnIndexData { + absolute: 0, + relative: 0, + relative_offset: 0, + } + } +} + +impl TableCellColumnIndexData { + /// Moves forward by `amount` columns, updating the various indices used + /// + /// This totally ignores rowspan -- if colspan and rowspan clash, + /// they just overlap, so we ignore it. + fn advance(&mut self, amount: u32, column_styles: &[ColumnStyle]) { + self.absolute += amount; + self.relative_offset += amount; + if let Some(mut current_col) = + column_styles.get(self.relative as usize) { + while self.relative_offset >= current_col.span { + // move to the next column + self.relative += 1; + self.relative_offset -= current_col.span; + if let Some(column_style) = + column_styles.get(self.relative as usize) { + current_col = column_style; + } else { + // we ran out of column_styles, + // so we don't need to update the indices + break; + } + } + } + } +} + impl<'table> Iterator for TableCellStyleIterator<'table> { type Item = TableCellStyleInfo<'table>; #[inline] fn next(&mut self) -> Option { - if let Some(ref mut row_info) = self.row_info { + // FIXME We do this awkward .take() followed by shoving it back in + // because without NLL the row_info borrow lasts too long + if let Some(mut row_info) = self.row_info.take() { if let Some(cell) = row_info.cell_iterator.next() { let rowgroup_style = row_info.rowgroup.map(|r| r.style()); - let row_style = row_info.row.style(); + let row_style = row_info.row.block_flow.fragment.style(); let cell = cell.as_table_cell(); let (col_style, colgroup_style) = if let Some(column_style) = - self.column_styles.get(self.column_index_relative as usize) { + self.column_styles.get(self.column_index.relative as usize) { let styles = (column_style.col_style.clone(), column_style.colgroup_style.clone()); - // FIXME incoming_rowspan - let cell_span = cell.column_span; + self.column_index.advance(cell.column_span, &self.column_styles); - let mut current_col = column_style; - self.column_index_relative_offset += cell_span; - while self.column_index_relative_offset >= current_col.span { - // move to the next column - self.column_index_relative += 1; - self.column_index_relative_offset -= current_col.span; - if let Some(column_style) = - self.column_styles.get(self.column_index_relative as usize) { - current_col = column_style; - } else { - break; - } - } styles } else { (None, None) }; - + // put row_info back in + self.row_info = Some(row_info); return Some(TableCellStyleInfo { cell, colgroup_style, @@ -1082,26 +1115,23 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { } else { // next row if let Some((group, row)) = self.row_iterator.next() { - *row_info = TableCellStyleIteratorRowInfo { - row: &row.block_flow.fragment, + self.row_info = Some(TableCellStyleIteratorRowInfo { + row: &row, rowgroup: group, cell_iterator: row.block_flow.base.child_iter() - }; - self.column_index_relative = 0; - self.column_index_relative_offset = 0; - // FIXME self.next() really should be up here but - // can't be without NLL, so instead it's at the - // end of the function + }); + self.column_index = Default::default(); + self.next() } else { // out of rows - return None + // row_info stays None + None } } } else { // empty table - return None + None } - self.next() } } From f796823b944e2434a14458283c980eeb3422ba8d Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 14 Feb 2018 17:17:34 -0800 Subject: [PATCH 15/20] Handle rowspan --- components/layout/table.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/components/layout/table.rs b/components/layout/table.rs index 36aa1d10cb8..269267010a6 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -1089,6 +1089,16 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { // FIXME We do this awkward .take() followed by shoving it back in // because without NLL the row_info borrow lasts too long if let Some(mut row_info) = self.row_info.take() { + if let Some(rowspan) = row_info.row.incoming_rowspan.get(self.column_index.absolute as usize) { + // we are not allowed to use this column as a starting point. Try the next one. + if *rowspan > 1 { + self.column_index.advance(1, &self.column_styles); + // put row_info back in + self.row_info = Some(row_info); + // try again + return self.next(); + } + } if let Some(cell) = row_info.cell_iterator.next() { let rowgroup_style = row_info.rowgroup.map(|r| r.style()); let row_style = row_info.row.block_flow.fragment.style(); From c2de3eb9accea7350bbd161e945aa85245e7f6fc Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 16 Feb 2018 13:49:42 -0800 Subject: [PATCH 16/20] Check visibility beforehand --- components/layout/table.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 269267010a6..fee95d1af7a 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -1147,7 +1147,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { impl<'table> TableCellStyleInfo<'table> { fn build_display_list(&self, mut state: &mut DisplayListBuildState) { - if !self.cell.visible { + if !self.cell.visible || self.cell.block_flow.fragment.style() + .get_inheritedbox().visibility != Visibility::Visible { return } let border_painting_mode = match self.cell.block_flow From 180b29ae66def2b471618b73557a9e7d163388fa Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 16 Feb 2018 13:56:52 -0800 Subject: [PATCH 17/20] Unconditionally remove repaint damage for table cells/rows/rgs --- components/layout/table.rs | 2 ++ components/layout/table_cell.rs | 8 +++----- components/layout/table_row.rs | 4 ++++ components/layout/table_rowgroup.rs | 9 ++++++--- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index fee95d1af7a..b8e491479ed 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -1147,6 +1147,8 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { impl<'table> TableCellStyleInfo<'table> { fn build_display_list(&self, mut state: &mut DisplayListBuildState) { + use style::computed_values::visibility::T as Visibility; + if !self.cell.visible || self.cell.block_flow.fragment.style() .get_inheritedbox().visibility != Visibility::Visible { return diff --git a/components/layout/table_cell.rs b/components/layout/table_cell.rs index 65e613dfbd2..f3bbe14f2e7 100644 --- a/components/layout/table_cell.rs +++ b/components/layout/table_cell.rs @@ -251,11 +251,9 @@ impl Flow for TableCellFlow { // This is handled by TableCellStyleInfo::build_display_list() // when the containing table builds its display list - if self.visible { - // we skip setting the damage in TableCellStyleInfo::build_display_list() - // because we only have immutable access - self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); - } + // we skip setting the damage in TableCellStyleInfo::build_display_list() + // because we only have immutable access + self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { diff --git a/components/layout/table_row.rs b/components/layout/table_row.rs index be83f6321e3..75fff53a969 100644 --- a/components/layout/table_row.rs +++ b/components/layout/table_row.rs @@ -467,7 +467,11 @@ impl Flow for TableRowFlow { } fn build_display_list(&mut self, _: &mut DisplayListBuildState) { + use style::servo::restyle_damage::ServoRestyleDamage; // handled in TableCellStyleInfo::build_display_list + // we skip setting the damage in TableCellStyleInfo::build_display_list() + // because we only have immutable access + self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { diff --git a/components/layout/table_rowgroup.rs b/components/layout/table_rowgroup.rs index 7ce2205c745..49b0d427084 100644 --- a/components/layout/table_rowgroup.rs +++ b/components/layout/table_rowgroup.rs @@ -176,9 +176,12 @@ impl Flow for TableRowGroupFlow { self.block_flow.update_late_computed_block_position_if_necessary(block_position) } - fn build_display_list(&mut self, state: &mut DisplayListBuildState) { - debug!("build_display_list_table_rowgroup: same process as block flow"); - self.block_flow.build_display_list(state); + fn build_display_list(&mut self, _: &mut DisplayListBuildState) { + use style::servo::restyle_damage::ServoRestyleDamage; + + // we skip setting the damage in TableCellStyleInfo::build_display_list() + // because we only have immutable access + self.block_flow.fragment.restyle_damage.remove(ServoRestyleDamage::REPAINT); } fn collect_stacking_contexts(&mut self, state: &mut StackingContextCollectionState) { From c36335e03142013e66f66084a2c290ac885d7408 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 16 Feb 2018 13:57:51 -0800 Subject: [PATCH 18/20] Remove table_style; tables already get their backgrounds painted correctly --- components/layout/table.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index b8e491479ed..42c759cee63 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -993,7 +993,6 @@ struct TableCellStyleIterator<'table> { column_styles: Vec>, row_iterator: TableRowAndGroupIterator<'table>, row_info: Option>, - table_style: &'table ComputedValues, column_index: TableCellColumnIndexData, } @@ -1020,14 +1019,12 @@ impl<'table> TableCellStyleIterator<'table> { TableCellStyleIterator { column_styles, row_iterator, row_info, column_index: Default::default(), - table_style: table.block_flow.fragment.style(), } } } struct TableCellStyleInfo<'table> { cell: &'table TableCellFlow, - table_style: &'table ComputedValues, colgroup_style: Option<&'table ComputedValues>, col_style: Option<&'table ComputedValues>, rowgroup_style: Option<&'table ComputedValues>, @@ -1120,7 +1117,6 @@ impl<'table> Iterator for TableCellStyleIterator<'table> { col_style, rowgroup_style, row_style, - table_style: self.table_style, }) } else { // next row @@ -1179,9 +1175,6 @@ impl<'table> TableCellStyleInfo<'table> { sty_ptr = sty as *const _; }; - - build_dl(self.table_style, &mut state); - if let Some(ref sty) = self.colgroup_style { build_dl(&sty, &mut state); } From 7de29d0ac4dde6f002bc124884374c9aa0883c9b Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 16 Feb 2018 15:59:17 -0800 Subject: [PATCH 19/20] Update test expectations --- .../backgrounds/background-attachment-applies-to-001.xht.ini | 2 ++ .../backgrounds/background-attachment-applies-to-002.xht.ini | 2 ++ .../backgrounds/background-attachment-applies-to-003.xht.ini | 2 ++ .../backgrounds/background-attachment-applies-to-004.xht.ini | 2 ++ .../CSS2/backgrounds/background-image-applies-to-001.xht.ini | 2 ++ .../CSS2/backgrounds/background-image-applies-to-002.xht.ini | 2 ++ .../CSS2/backgrounds/background-image-applies-to-003.xht.ini | 2 ++ .../CSS2/backgrounds/background-image-applies-to-004.xht.ini | 2 ++ .../backgrounds/background-position-applies-to-001.xht.ini | 2 ++ .../backgrounds/background-position-applies-to-002.xht.ini | 2 ++ .../backgrounds/background-position-applies-to-003.xht.ini | 2 ++ .../backgrounds/background-position-applies-to-004.xht.ini | 2 ++ .../CSS2/backgrounds/background-repeat-applies-to-001.xht.ini | 2 ++ .../CSS2/backgrounds/background-repeat-applies-to-002.xht.ini | 2 ++ .../CSS2/backgrounds/background-repeat-applies-to-003.xht.ini | 2 ++ .../CSS2/backgrounds/background-repeat-applies-to-004.xht.ini | 2 ++ .../css/CSS2/bidi-text/direction-applies-to-001.xht.ini | 2 ++ .../css/CSS2/bidi-text/direction-applies-to-002.xht.ini | 2 ++ .../css/CSS2/bidi-text/direction-applies-to-003.xht.ini | 2 ++ .../css/CSS2/bidi-text/direction-applies-to-004.xht.ini | 2 ++ tests/wpt/metadata/css/CSS2/box-display/display-012.xht.ini | 3 ++- .../css/CSS2/selectors/default-attribute-selector-004.xht.ini | 3 --- .../attachment-local-clipping-color-4.html.ini | 3 --- 23 files changed, 42 insertions(+), 7 deletions(-) create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-001.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-002.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-003.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-004.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-001.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-002.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-003.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-004.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-001.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-002.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-003.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-004.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-001.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-002.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-003.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-004.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-001.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-002.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-003.xht.ini create mode 100644 tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-004.xht.ini delete mode 100644 tests/wpt/metadata/css/CSS2/selectors/default-attribute-selector-004.xht.ini delete mode 100644 tests/wpt/metadata/css/css-backgrounds/background-attachment-local/attachment-local-clipping-color-4.html.ini diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-001.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-001.xht.ini new file mode 100644 index 00000000000..f999f5864f3 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-001.xht.ini @@ -0,0 +1,2 @@ +[background-attachment-applies-to-001.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-002.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-002.xht.ini new file mode 100644 index 00000000000..53b06df4e84 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-002.xht.ini @@ -0,0 +1,2 @@ +[background-attachment-applies-to-002.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-003.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-003.xht.ini new file mode 100644 index 00000000000..6b236561a0f --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-003.xht.ini @@ -0,0 +1,2 @@ +[background-attachment-applies-to-003.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-004.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-004.xht.ini new file mode 100644 index 00000000000..d53f576333a --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-attachment-applies-to-004.xht.ini @@ -0,0 +1,2 @@ +[background-attachment-applies-to-004.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-001.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-001.xht.ini new file mode 100644 index 00000000000..83447c501ce --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-001.xht.ini @@ -0,0 +1,2 @@ +[background-image-applies-to-001.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-002.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-002.xht.ini new file mode 100644 index 00000000000..4aeaf88f3ef --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-002.xht.ini @@ -0,0 +1,2 @@ +[background-image-applies-to-002.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-003.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-003.xht.ini new file mode 100644 index 00000000000..5ef1b8ae866 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-003.xht.ini @@ -0,0 +1,2 @@ +[background-image-applies-to-003.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-004.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-004.xht.ini new file mode 100644 index 00000000000..58609296e43 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-image-applies-to-004.xht.ini @@ -0,0 +1,2 @@ +[background-image-applies-to-004.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-001.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-001.xht.ini new file mode 100644 index 00000000000..f4a302c54c5 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-001.xht.ini @@ -0,0 +1,2 @@ +[background-position-applies-to-001.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-002.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-002.xht.ini new file mode 100644 index 00000000000..c39568f745d --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-002.xht.ini @@ -0,0 +1,2 @@ +[background-position-applies-to-002.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-003.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-003.xht.ini new file mode 100644 index 00000000000..157ef0fbd5e --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-003.xht.ini @@ -0,0 +1,2 @@ +[background-position-applies-to-003.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-004.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-004.xht.ini new file mode 100644 index 00000000000..e2126dc0757 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-position-applies-to-004.xht.ini @@ -0,0 +1,2 @@ +[background-position-applies-to-004.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-001.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-001.xht.ini new file mode 100644 index 00000000000..a356c6caccb --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-001.xht.ini @@ -0,0 +1,2 @@ +[background-repeat-applies-to-001.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-002.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-002.xht.ini new file mode 100644 index 00000000000..00f88e65b4f --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-002.xht.ini @@ -0,0 +1,2 @@ +[background-repeat-applies-to-002.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-003.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-003.xht.ini new file mode 100644 index 00000000000..d8c00b77035 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-003.xht.ini @@ -0,0 +1,2 @@ +[background-repeat-applies-to-003.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-004.xht.ini b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-004.xht.ini new file mode 100644 index 00000000000..ae6baf543c0 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/backgrounds/background-repeat-applies-to-004.xht.ini @@ -0,0 +1,2 @@ +[background-repeat-applies-to-004.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-001.xht.ini b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-001.xht.ini new file mode 100644 index 00000000000..f7f7d23d54e --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-001.xht.ini @@ -0,0 +1,2 @@ +[direction-applies-to-001.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-002.xht.ini b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-002.xht.ini new file mode 100644 index 00000000000..75eb44de5d4 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-002.xht.ini @@ -0,0 +1,2 @@ +[direction-applies-to-002.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-003.xht.ini b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-003.xht.ini new file mode 100644 index 00000000000..b605bc11df2 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-003.xht.ini @@ -0,0 +1,2 @@ +[direction-applies-to-003.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-004.xht.ini b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-004.xht.ini new file mode 100644 index 00000000000..ae6097070f9 --- /dev/null +++ b/tests/wpt/metadata/css/CSS2/bidi-text/direction-applies-to-004.xht.ini @@ -0,0 +1,2 @@ +[direction-applies-to-004.xht] + expected: FAIL diff --git a/tests/wpt/metadata/css/CSS2/box-display/display-012.xht.ini b/tests/wpt/metadata/css/CSS2/box-display/display-012.xht.ini index 118939a6dc3..8028068bd12 100644 --- a/tests/wpt/metadata/css/CSS2/box-display/display-012.xht.ini +++ b/tests/wpt/metadata/css/CSS2/box-display/display-012.xht.ini @@ -1,3 +1,4 @@ [display-012.xht] type: reftest - expected: FAIL + expected: + if os == "mac": FAIL # Fonts on OSX make this gain an additional pixel of background (also fails on Firefox) diff --git a/tests/wpt/metadata/css/CSS2/selectors/default-attribute-selector-004.xht.ini b/tests/wpt/metadata/css/CSS2/selectors/default-attribute-selector-004.xht.ini deleted file mode 100644 index 63ccce31e49..00000000000 --- a/tests/wpt/metadata/css/CSS2/selectors/default-attribute-selector-004.xht.ini +++ /dev/null @@ -1,3 +0,0 @@ -[default-attribute-selector-004.xht] - type: reftest - expected: FAIL diff --git a/tests/wpt/metadata/css/css-backgrounds/background-attachment-local/attachment-local-clipping-color-4.html.ini b/tests/wpt/metadata/css/css-backgrounds/background-attachment-local/attachment-local-clipping-color-4.html.ini deleted file mode 100644 index de5eaf2f86f..00000000000 --- a/tests/wpt/metadata/css/css-backgrounds/background-attachment-local/attachment-local-clipping-color-4.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[attachment-local-clipping-color-4.html] - type: reftest - expected: FAIL From 285313f3c7a528bfde8df567262e3f9f4df61814 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 21 Feb 2018 00:36:35 -0800 Subject: [PATCH 20/20] bail early for initial valued backgrounds --- components/layout/table.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/components/layout/table.rs b/components/layout/table.rs index 42c759cee63..72916de36ed 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -19,11 +19,12 @@ use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use gfx_traits::print_tree::PrintTree; use layout_debug; use model::{IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto}; -use std::{cmp, fmt, ptr}; +use std::{cmp, fmt}; use style::computed_values::{border_collapse, border_spacing, table_layout}; use style::context::SharedStyleContext; use style::logical_geometry::LogicalSize; use style::properties::ComputedValues; +use style::properties::style_structs::Background; use style::servo::restyle_damage::ServoRestyleDamage; use style::values::CSSFloat; use style::values::computed::LengthOrPercentageOrAuto; @@ -1159,20 +1160,18 @@ impl<'table> TableCellStyleInfo<'table> { }; { let cell_flow = &self.cell.block_flow; - let mut sty_ptr = ptr::null(); + let initial = ComputedValues::initial_values(); - let mut build_dl = |sty: &ComputedValues, state: &mut &mut DisplayListBuildState| { + let build_dl = |sty: &ComputedValues, state: &mut &mut DisplayListBuildState| { + let background = sty.get_background(); // Don't redraw backgrounds that we've already drawn - if sty_ptr == sty as *const _ { + if background as *const Background == initial.get_background() as *const _ { return; } - let background = sty.get_background(); let background_color = sty.resolve_color(background.background_color); cell_flow.build_display_list_for_background_if_applicable_with_background( state, background, background_color ); - - sty_ptr = sty as *const _; }; if let Some(ref sty) = self.colgroup_style {