From 6c684a5ac70490141460084a6b19e7e27728e45a Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Mon, 29 Feb 2016 14:33:41 -0800 Subject: [PATCH] Fix border collapsing at the end of a table-row-group This fixes the border-end calculation for table rows whose borders are collapsed with rows in different rowgroups. The border collapsing code now uses an iterator that yields all the rows as a flat sequence, regardless of how they are grouped in rowgroups. It gets rid of `TableRowGroupFlow::preliminary_collapsed_borders` which was never correct. (It was read but never written.) This may fix #8120 but I'm not 100% certain. (I haven't managed to reproduce the intermittent failure locally, and my reduced test case still fails but in a different way.) --- components/layout/table.rs | 228 ++++++++---------- components/layout/table_rowgroup.rs | 7 +- .../border-collapse-dynamic-cell-002.htm.ini | 3 - tests/wpt/mozilla/meta/MANIFEST.json | 24 ++ .../css/border_collapse_simple_a.html.ini | 3 - .../tests/css/border_collapse_rowgroup_a.html | 28 +++ .../css/border_collapse_rowgroup_ref.html | 27 +++ 7 files changed, 181 insertions(+), 139 deletions(-) delete mode 100644 tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-cell-002.htm.ini delete mode 100644 tests/wpt/mozilla/meta/css/border_collapse_simple_a.html.ini create mode 100644 tests/wpt/mozilla/tests/css/border_collapse_rowgroup_a.html create mode 100644 tests/wpt/mozilla/tests/css/border_collapse_rowgroup_ref.html diff --git a/components/layout/table.rs b/components/layout/table.rs index 15decaed28a..7f0a4b09a59 100644 --- a/components/layout/table.rs +++ b/components/layout/table.rs @@ -12,8 +12,9 @@ use block::{ISizeConstraintInput, ISizeConstraintSolution}; use context::LayoutContext; use display_list_builder::{BlockFlowDisplayListBuilding, BorderPaintingMode}; use euclid::Point2D; -use flow::{IMPACTED_BY_RIGHT_FLOATS, ImmutableFlowUtils, OpaqueFlow}; +use flow::{BaseFlow, IMPACTED_BY_RIGHT_FLOATS, ImmutableFlowUtils, OpaqueFlow}; use flow::{self, EarlyAbsolutePositionInfo, Flow, FlowClass, IMPACTED_BY_LEFT_FLOATS}; +use flow_list::MutFlowListIterator; use fragment::{Fragment, FragmentBorderBoxIterator, Overflow}; use gfx::display_list::DisplayList; use incremental::{REFLOW, REFLOW_OUT_OF_FLOW}; @@ -133,7 +134,7 @@ impl TableFlow { /// Updates the minimum and preferred inline-size calculation for a single row. This is /// factored out into a separate function because we process children of rowgroups too. - fn update_column_inline_sizes_for_row(child: &mut Flow, + fn update_column_inline_sizes_for_row(row: &mut TableRowFlow, column_inline_sizes: &mut Vec, computation: &mut IntrinsicISizesContribution, first_row: bool, @@ -142,8 +143,6 @@ impl TableFlow { // not defined in the column group. // // FIXME: Need to read inline-sizes from either table-header-group OR the first table-row. - debug_assert!(child.is_table_row()); - let row = child.as_table_row(); match table_layout { TableLayout::Fixed => { // Fixed table layout only looks at the first row. @@ -230,6 +229,28 @@ impl Flow for TableFlow { // Don't use `compute_intrinsic_inline_sizes` here because that will count padding as // part of the table, which we don't want to do—it belongs to the table wrapper instead. + // Get column inline sizes from colgroups + for kid in self.block_flow.base.child_iter().filter(|kid| kid.is_table_colgroup()) { + for specified_inline_size in &kid.as_mut_table_colgroup().inline_sizes { + self.column_intrinsic_inline_sizes.push(ColumnIntrinsicInlineSize { + minimum_length: match *specified_inline_size { + LengthOrPercentageOrAuto::Auto | + LengthOrPercentageOrAuto::Calc(_) | + LengthOrPercentageOrAuto::Percentage(_) => Au(0), + LengthOrPercentageOrAuto::Length(length) => length, + }, + percentage: match *specified_inline_size { + LengthOrPercentageOrAuto::Auto | + LengthOrPercentageOrAuto::Calc(_) | + LengthOrPercentageOrAuto::Length(_) => 0.0, + LengthOrPercentageOrAuto::Percentage(percentage) => percentage, + }, + preferred: Au(0), + constrained: false, + }) + } + } + self.collapsed_inline_direction_border_widths_for_table = Vec::new(); self.collapsed_block_direction_border_widths_for_table = vec![Au(0)]; @@ -250,133 +271,47 @@ impl Flow for TableFlow { }; let mut computation = IntrinsicISizesContribution::new(); - let mut previous_collapsed_block_end_borders = if collapsing_borders { + let mut previous_collapsed_block_end_borders = PreviousBlockCollapsedBorders::FromTable(CollapsedBorder::block_start( &*self.block_flow.fragment.style, - CollapsedBorderProvenance::FromTable)) - } else { - PreviousBlockCollapsedBorders::NotCollapsingBorders - }; + CollapsedBorderProvenance::FromTable)); let mut first_row = true; { - let mut iterator = self.block_flow.base.child_iter().peekable(); - while let Some(kid) = iterator.next() { - if kid.is_table_colgroup() { - for specified_inline_size in &kid.as_mut_table_colgroup().inline_sizes { - self.column_intrinsic_inline_sizes.push(ColumnIntrinsicInlineSize { - minimum_length: match *specified_inline_size { - LengthOrPercentageOrAuto::Auto | - LengthOrPercentageOrAuto::Calc(_) | - LengthOrPercentageOrAuto::Percentage(_) => Au(0), - LengthOrPercentageOrAuto::Length(length) => length, - }, - percentage: match *specified_inline_size { - LengthOrPercentageOrAuto::Auto | - LengthOrPercentageOrAuto::Calc(_) | - LengthOrPercentageOrAuto::Length(_) => 0.0, - LengthOrPercentageOrAuto::Percentage(percentage) => percentage, - }, - preferred: Au(0), - constrained: false, - }) - } - } else if kid.is_table_row() { - TableFlow::update_column_inline_sizes_for_row( - kid, - &mut self.column_intrinsic_inline_sizes, - &mut computation, - first_row, - self.table_layout); - if collapsing_borders { - let next_index_and_sibling = iterator.peek(); - let next_collapsed_borders_in_block_direction = - match next_index_and_sibling { - Some(next_sibling) => { - if next_sibling.is_table_rowgroup() { - NextBlockCollapsedBorders::FromNextRow( - &next_sibling.as_table_rowgroup() - .preliminary_collapsed_borders - .block_start) - } else { - NextBlockCollapsedBorders::FromNextRow( - &next_sibling.as_table_row() - .preliminary_collapsed_borders - .block_start) - } - } - None => { - NextBlockCollapsedBorders::FromTable( - CollapsedBorder::block_end(&*self.block_flow.fragment.style, - CollapsedBorderProvenance::FromTable)) - } - }; - perform_border_collapse_for_row( - kid.as_mut_table_row(), - table_inline_collapsed_borders.as_ref().unwrap(), - previous_collapsed_block_end_borders, - next_collapsed_borders_in_block_direction, - &mut self.collapsed_inline_direction_border_widths_for_table, - &mut self.collapsed_block_direction_border_widths_for_table); - previous_collapsed_block_end_borders = - PreviousBlockCollapsedBorders::FromPreviousRow( - kid.as_table_row().final_collapsed_borders.block_end.clone()) - } - first_row = false - } else if kid.is_table_rowgroup() { - let mut iterator = flow::mut_base(kid).child_iter().peekable(); - while let Some(grandkid) = iterator.next() { - let grandkid_next_sibling = iterator.peek(); - let next_collapsed_borders_in_block_direction = if collapsing_borders { - match grandkid_next_sibling { - Some(grandkid_next_sibling) => { - if grandkid_next_sibling.is_table_rowgroup() { - NextBlockCollapsedBorders::FromNextRow( - &grandkid_next_sibling.as_table_rowgroup() - .preliminary_collapsed_borders - .block_start) - } else { - NextBlockCollapsedBorders::FromNextRow( - &grandkid_next_sibling.as_table_row() - .preliminary_collapsed_borders - .block_start) - } - } - None => { - NextBlockCollapsedBorders::FromTable( - CollapsedBorder::block_end( - &*self.block_flow.fragment.style, - CollapsedBorderProvenance::FromTable)) - } + let mut iterator = TableRowIterator::new(&mut self.block_flow.base).peekable(); + while let Some(row) = iterator.next() { + TableFlow::update_column_inline_sizes_for_row(row, + &mut self.column_intrinsic_inline_sizes, + &mut computation, + first_row, + self.table_layout); + if collapsing_borders { + let next_index_and_sibling = iterator.peek(); + let next_collapsed_borders_in_block_direction = + match next_index_and_sibling { + Some(next_sibling) => { + NextBlockCollapsedBorders::FromNextRow( + &next_sibling.as_table_row() + .preliminary_collapsed_borders + .block_start) + } + None => { + NextBlockCollapsedBorders::FromTable( + CollapsedBorder::block_end(&*self.block_flow.fragment.style, + CollapsedBorderProvenance::FromTable)) } - } else { - NextBlockCollapsedBorders::NotCollapsingBorders }; - - TableFlow::update_column_inline_sizes_for_row( - grandkid, - &mut self.column_intrinsic_inline_sizes, - &mut computation, - first_row, - self.table_layout); - if collapsing_borders { - perform_border_collapse_for_row( - grandkid.as_mut_table_row(), - table_inline_collapsed_borders.as_ref().unwrap(), - previous_collapsed_block_end_borders, - next_collapsed_borders_in_block_direction, - &mut self.collapsed_inline_direction_border_widths_for_table, - &mut self.collapsed_block_direction_border_widths_for_table); - previous_collapsed_block_end_borders = - PreviousBlockCollapsedBorders::FromPreviousRow( - grandkid.as_table_row() - .final_collapsed_borders - .block_end - .clone()) - } - first_row = false - } + perform_border_collapse_for_row(row, + table_inline_collapsed_borders.as_ref().unwrap(), + previous_collapsed_block_end_borders, + next_collapsed_borders_in_block_direction, + &mut self.collapsed_inline_direction_border_widths_for_table, + &mut self.collapsed_block_direction_border_widths_for_table); + previous_collapsed_block_end_borders = + PreviousBlockCollapsedBorders::FromPreviousRow( + row.final_collapsed_borders.block_end.clone()) } + first_row = false } } @@ -723,7 +658,6 @@ fn perform_border_collapse_for_row(child_table_row: &mut TableRowFlow, child_table_row.final_collapsed_borders.block_start = vec![collapsed_border; child_table_row.block_flow.base.children.len()] } - PreviousBlockCollapsedBorders::NotCollapsingBorders => {} } // Compute block-end borders. @@ -744,7 +678,6 @@ fn perform_border_collapse_for_row(child_table_row: &mut TableRowFlow, NextBlockCollapsedBorders::FromTable(ref next_block_borders) => { next_block.combine(next_block_borders); } - NextBlockCollapsedBorders::NotCollapsingBorders => {} } *block_spacing = cmp::max(*block_spacing, next_block.width) } @@ -851,11 +784,52 @@ struct TableInlineCollapsedBorders { enum PreviousBlockCollapsedBorders { FromPreviousRow(Vec), FromTable(CollapsedBorder), - NotCollapsingBorders, } enum NextBlockCollapsedBorders<'a> { FromNextRow(&'a [CollapsedBorder]), FromTable(CollapsedBorder), - NotCollapsingBorders, +} + +/// Iterator over all the rows of a table +struct TableRowIterator<'a> { + kids: MutFlowListIterator<'a>, + grandkids: Option>, +} + +impl<'a> TableRowIterator<'a> { + fn new(base: &'a mut BaseFlow) -> Self { + TableRowIterator { + kids: base.child_iter(), + grandkids: None, + } + } +} + +impl<'a> Iterator for TableRowIterator<'a> { + type Item = &'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()) + } + } + // Otherwise, iterate through the table's children. + self.grandkids = None; + match self.kids.next() { + Some(kid) => { + if kid.is_table_rowgroup() { + self.grandkids = Some(flow::mut_base(kid).child_iter()); + self.next() + } else if kid.is_table_row() { + Some(kid.as_mut_table_row()) + } else { + self.next() // Skip children that are not rows or rowgroups + } + } + None => None + } + } } diff --git a/components/layout/table_rowgroup.rs b/components/layout/table_rowgroup.rs index 6679b01fe2f..4536f96055e 100644 --- a/components/layout/table_rowgroup.rs +++ b/components/layout/table_rowgroup.rs @@ -21,7 +21,7 @@ use style::computed_values::{border_collapse, border_spacing}; use style::logical_geometry::{LogicalSize, WritingMode}; use style::properties::ComputedValues; use table::{ColumnComputedInlineSize, ColumnIntrinsicInlineSize, InternalTable, TableLikeFlow}; -use table_row::{self, CollapsedBordersForRow}; +use table_row; use util::print_tree::PrintTree; /// A table formatting context. @@ -42,10 +42,6 @@ pub struct TableRowGroupFlow { /// assignment phase. pub table_writing_mode: WritingMode, - /// Information about the borders for each cell that we bubble up to our parent. This is only - /// computed if `border-collapse` is `collapse`. - pub preliminary_collapsed_borders: CollapsedBordersForRow, - /// The final width of the borders in the inline direction for each cell, computed by the /// entire table and pushed down into each row during inline size computation. pub collapsed_inline_direction_border_widths_for_table: Vec, @@ -73,7 +69,6 @@ impl TableRowGroupFlow { vertical: Au(0), }, table_writing_mode: writing_mode, - preliminary_collapsed_borders: CollapsedBordersForRow::new(), collapsed_inline_direction_border_widths_for_table: Vec::new(), collapsed_block_direction_border_widths_for_table: Vec::new(), } diff --git a/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-cell-002.htm.ini b/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-cell-002.htm.ini deleted file mode 100644 index fcae447d8ba..00000000000 --- a/tests/wpt/metadata-css/css21_dev/html4/border-collapse-dynamic-cell-002.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[border-collapse-dynamic-cell-002.htm] - type: reftest - expected: FAIL diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index b12f912b49e..9c44589a96d 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -740,6 +740,18 @@ "url": "/_mozilla/css/border_collapse_missing_cell_a.html" } ], + "css/border_collapse_rowgroup_a.html": [ + { + "path": "css/border_collapse_rowgroup_a.html", + "references": [ + [ + "/_mozilla/css/border_collapse_rowgroup_ref.html", + "==" + ] + ], + "url": "/_mozilla/css/border_collapse_rowgroup_a.html" + } + ], "css/border_collapse_simple_a.html": [ { "path": "css/border_collapse_simple_a.html", @@ -6910,6 +6922,18 @@ "url": "/_mozilla/css/border_collapse_missing_cell_a.html" } ], + "css/border_collapse_rowgroup_a.html": [ + { + "path": "css/border_collapse_rowgroup_a.html", + "references": [ + [ + "/_mozilla/css/border_collapse_rowgroup_ref.html", + "==" + ] + ], + "url": "/_mozilla/css/border_collapse_rowgroup_a.html" + } + ], "css/border_collapse_simple_a.html": [ { "path": "css/border_collapse_simple_a.html", diff --git a/tests/wpt/mozilla/meta/css/border_collapse_simple_a.html.ini b/tests/wpt/mozilla/meta/css/border_collapse_simple_a.html.ini deleted file mode 100644 index 4812e11a523..00000000000 --- a/tests/wpt/mozilla/meta/css/border_collapse_simple_a.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[border_collapse_simple_a.html] - type: reftest - disabled: https://github.com/servo/servo/issues/8120 diff --git a/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_a.html b/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_a.html new file mode 100644 index 00000000000..e5f010d5cd5 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_a.html @@ -0,0 +1,28 @@ + + + + + border collapse rowgroup test + + + + + + + + + +
+ + diff --git a/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_ref.html b/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_ref.html new file mode 100644 index 00000000000..7398716ef8e --- /dev/null +++ b/tests/wpt/mozilla/tests/css/border_collapse_rowgroup_ref.html @@ -0,0 +1,27 @@ + + + + + border collapse rowgroup reference + + + + + + + + +
+ +