servo: Merge #5233 - Fixes for positioning of RTL blocks (from mbrubeck:rtl-position); r=SimonSapin
authorMatt Brubeck <mbrubeck@limpet.net>
Tue, 17 Mar 2015 12:33:53 -0600
changeset 335929 b3be51cfa6c1589ed78eb34804efc18b1f22d157
parent 335928 29da0aa8040ea26e05dc1efa852edf2e0ba55d16
child 335930 79740624c87364c2640890342e0bdf3384666638
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersSimonSapin
servo: Merge #5233 - Fixes for positioning of RTL blocks (from mbrubeck:rtl-position); r=SimonSapin This fixes a bug in finding the top left corner of an RTL block in physical coordinates. (The old code used the `start` point of the `position` rect, which is not always the top left.) It also fixes the setting of `position.start.i` in certain mixed LTR/RTL cases. There is still a bug related to `position.size` for RTL blocks with margins. See the FIXME comments for details. r? @pcwalton or @SimonSapin Source-Repo: https://github.com/servo/servo Source-Revision: b255b49e2e2246ff8bf7f8751088bfe0a0ee41a2
servo/components/layout/block.rs
servo/components/layout/table.rs
servo/components/layout/table_cell.rs
servo/components/layout/table_row.rs
servo/components/layout/table_rowgroup.rs
servo/components/layout/table_wrapper.rs
--- a/servo/components/layout/block.rs
+++ b/servo/components/layout/block.rs
@@ -1232,16 +1232,17 @@ impl BlockFlow {
     ///
     /// `#[inline(always)]` because this is called only from block or table inline-size assignment
     /// and the code for block layout is significantly simpler.
     #[inline(always)]
     pub fn propagate_assigned_inline_size_to_children(
             &mut self,
             layout_context: &LayoutContext,
             inline_start_content_edge: Au,
+            inline_end_content_edge: Au,
             content_inline_size: Au,
             table_info: Option<table::ChildInlineSizeInfo>) {
         // Keep track of whether floats could impact each child.
         let mut inline_start_floats_impact_child =
             self.base.flags.contains(IMPACTED_BY_LEFT_FLOATS);
         let mut inline_end_floats_impact_child =
             self.base.flags.contains(IMPACTED_BY_RIGHT_FLOATS);
 
@@ -1335,29 +1336,29 @@ impl BlockFlow {
                 let kid_base = flow::mut_base(kid);
                 if !kid_base.flags.contains(IS_ABSOLUTELY_POSITIONED) &&
                         !kid_base.flags.is_float() {
                     kid_base.position.start.i =
                         if kid_mode.is_bidi_ltr() == containing_block_mode.is_bidi_ltr() {
                             inline_start_content_edge
                         } else {
                             // The kid's inline 'start' is at the parent's 'end'
-                            inline_start_content_edge + content_inline_size
-                        }
+                            inline_end_content_edge
+                        };
                 }
                 kid_base.block_container_inline_size = content_inline_size;
                 kid_base.block_container_writing_mode = containing_block_mode;
             }
             if kid.is_block_like() {
                 kid.as_block().hypothetical_position.i =
                     if kid_mode.is_bidi_ltr() == containing_block_mode.is_bidi_ltr() {
                         inline_start_content_edge
                     } else {
                         // The kid's inline 'start' is at the parent's 'end'
-                        inline_start_content_edge + content_inline_size
+                        inline_end_content_edge
                     }
             }
 
             // Determine float impaction.
             if flow::base(kid).flags.contains(CLEARS_LEFT) {
                 inline_start_floats_impact_child = false;
                 inline_size_of_preceding_left_floats = Au(0);
             }
@@ -1625,25 +1626,35 @@ impl Flow for BlockFlow {
                     self.inline_size_of_preceding_left_floats -
                     self.inline_size_of_preceding_right_floats;
             }
             FormattingContextType::Other => {
                 self.base.flags.remove(IMPACTED_BY_LEFT_FLOATS);
                 self.base.flags.remove(IMPACTED_BY_RIGHT_FLOATS);
             }
         }
-
         // Move in from the inline-start border edge.
         let inline_start_content_edge = self.fragment.border_box.start.i +
             self.fragment.border_padding.inline_start;
+
         let padding_and_borders = self.fragment.border_padding.inline_start_end();
+
+        // Distance from the inline-end margin edge to the inline-end content edge.
+        let inline_end_content_edge =
+            self.base.block_container_inline_size -
+            self.fragment.margin.inline_end -
+            self.fragment.border_box.size.inline -
+            self.fragment.border_box.start.i -
+            padding_and_borders;
+
         let content_inline_size = self.fragment.border_box.size.inline - padding_and_borders;
 
         self.propagate_assigned_inline_size_to_children(layout_context,
                                                         inline_start_content_edge,
+                                                        inline_end_content_edge,
                                                         content_inline_size,
                                                         None);
     }
 
     fn place_float_if_applicable<'a>(&mut self, _: &'a LayoutContext<'a>) {
         if self.base.flags.is_float() {
             self.place_float();
         }
@@ -1815,19 +1826,21 @@ impl Flow for BlockFlow {
                                               CoordinateSystem::Self);
         let clip = self.fragment.clipping_region_for_children(&clip_in_child_coordinate_system,
                                                               &stacking_relative_border_box);
 
         // Process children.
         for kid in self.base.child_iter() {
             if !flow::base(kid).flags.contains(IS_ABSOLUTELY_POSITIONED) {
                 let kid_base = flow::mut_base(kid);
-                kid_base.stacking_relative_position = origin_for_children +
-                    kid_base.position.start.to_physical(kid_base.writing_mode,
-                                                        container_size_for_children);
+                // FIXME (mbrubeck): `position.size` is inflated by the inline margin size, making
+                // this incorrect for RTL blocks (see `set_inline_size_constraint_solutions`).
+                let physical_position = kid_base.position.to_physical(kid_base.writing_mode,
+                                                                      container_size_for_children);
+                kid_base.stacking_relative_position = origin_for_children + physical_position.origin;
             }
 
             flow::mut_base(kid).absolute_position_info = absolute_position_info_for_children;
             flow::mut_base(kid).clip = clip.clone()
         }
     }
 
     fn mark_as_root(&mut self) {
@@ -2104,16 +2117,19 @@ pub trait ISizeAndMarginsComputer {
             // larger at all by the margin.
             extra_inline_size_from_margin = max(Au(0), fragment.margin.inline_start) +
                                             max(Au(0), fragment.margin.inline_end);
         }
 
         // We also resize the block itself, to ensure that overflow is not calculated
         // as the inline-size of our parent. We might be smaller and we might be larger if we
         // overflow.
+        //
+        // FIXME (mbrubeck): The margin is included in position.size but not position.start, which
+        // throws off position.to_physical results (especially for RTL blocks).
         flow::mut_base(block).position.size.inline = inline_size + extra_inline_size_from_margin;
     }
 
     /// Set the x coordinate of the given flow if it is absolutely positioned.
     fn set_flow_x_coord_if_necessary(&self, _: &mut BlockFlow, _: ISizeConstraintSolution) {}
 
     /// Solve the inline-size and margins constraints for this block flow.
     fn solve_inline_size_constraints(&self,
--- a/servo/components/layout/table.rs
+++ b/servo/components/layout/table.rs
@@ -295,16 +295,17 @@ impl Flow for TableFlow {
         }
 
         let inline_size_computer = InternalTable;
         inline_size_computer.compute_used_inline_size(&mut self.block_flow,
                                                       layout_context,
                                                       containing_block_inline_size);
 
         let inline_start_content_edge = self.block_flow.fragment.border_padding.inline_start;
+        let inline_end_content_edge = self.block_flow.fragment.border_padding.inline_end;
         let padding_and_borders = self.block_flow.fragment.border_padding.inline_start_end();
         let spacing_per_cell = self.spacing();
         let spacing = spacing_per_cell.horizontal *
             (self.column_intrinsic_inline_sizes.len() as i32 + 1);
         let content_inline_size =
             self.block_flow.fragment.border_box.size.inline - padding_and_borders - spacing;
 
         match self.table_layout {
@@ -347,16 +348,17 @@ impl Flow for TableFlow {
         self.block_flow.base.flags.remove(IMPACTED_BY_RIGHT_FLOATS);
 
         let info = ChildInlineSizeInfo {
             column_computed_inline_sizes: self.column_computed_inline_sizes.as_slice(),
             spacing: spacing_per_cell,
         };
         self.block_flow.propagate_assigned_inline_size_to_children(layout_context,
                                                                    inline_start_content_edge,
+                                                                   inline_end_content_edge,
                                                                    content_inline_size,
                                                                    Some(info));
     }
 
     fn assign_block_size<'a>(&mut self, layout_context: &'a LayoutContext<'a>) {
         debug!("assign_block_size: assigning block_size for table");
         let vertical_spacing = self.spacing().vertical;
         self.block_flow.assign_block_size_for_table_like_flow(layout_context, vertical_spacing)
--- a/servo/components/layout/table_cell.rs
+++ b/servo/components/layout/table_cell.rs
@@ -123,22 +123,27 @@ impl Flow for TableCellFlow {
 
         inline_size_computer.compute_used_inline_size(&mut self.block_flow,
                                                       layout_context,
                                                       containing_block_inline_size);
 
         let inline_start_content_edge =
             self.block_flow.fragment.border_box.start.i +
             self.block_flow.fragment.border_padding.inline_start;
+        let inline_end_content_edge =
+            self.block_flow.base.block_container_inline_size -
+            self.block_flow.fragment.border_padding.inline_start_end() -
+            self.block_flow.fragment.border_box.size.inline;
         let padding_and_borders = self.block_flow.fragment.border_padding.inline_start_end();
         let content_inline_size =
             self.block_flow.fragment.border_box.size.inline - padding_and_borders;
 
         self.block_flow.propagate_assigned_inline_size_to_children(layout_context,
                                                                    inline_start_content_edge,
+                                                                   inline_end_content_edge,
                                                                    content_inline_size,
                                                                    None);
     }
 
     fn assign_block_size<'a>(&mut self, ctx: &'a LayoutContext<'a>) {
         debug!("assign_block_size: assigning block_size for table_cell");
         self.assign_block_size_table_cell_base(ctx);
     }
--- a/servo/components/layout/table_row.rs
+++ b/servo/components/layout/table_row.rs
@@ -233,16 +233,17 @@ impl Flow for TableRowFlow {
                                          self.block_flow.base.debug_id());
         debug!("assign_inline_sizes({}): assigning inline_size for flow", "table_row");
 
         // The position was set to the containing block by the flow's parent.
         let containing_block_inline_size = self.block_flow.base.block_container_inline_size;
         // FIXME: In case of border-collapse: collapse, inline_start_content_edge should be
         // border_inline_start.
         let inline_start_content_edge = Au(0);
+        let inline_end_content_edge = Au(0);
 
         let inline_size_computer = InternalTable;
         inline_size_computer.compute_used_inline_size(&mut self.block_flow,
                                                       layout_context,
                                                       containing_block_inline_size);
 
         // Spread out the completed inline sizes among columns with spans > 1.
         let mut computed_inline_size_for_cells = Vec::new();
@@ -280,16 +281,17 @@ impl Flow for TableRowFlow {
 
         // Push those inline sizes down to the cells.
         let info = ChildInlineSizeInfo {
             column_computed_inline_sizes: computed_inline_size_for_cells.as_slice(),
             spacing: self.spacing,
         };
         self.block_flow.propagate_assigned_inline_size_to_children(layout_context,
                                                                    inline_start_content_edge,
+                                                                   inline_end_content_edge,
                                                                    containing_block_inline_size,
                                                                    Some(info));
     }
 
     fn assign_block_size<'a>(&mut self, ctx: &'a LayoutContext<'a>) {
         debug!("assign_block_size: assigning block_size for table_row");
         self.assign_block_size_table_row_base(ctx);
     }
--- a/servo/components/layout/table_rowgroup.rs
+++ b/servo/components/layout/table_rowgroup.rs
@@ -97,29 +97,31 @@ impl Flow for TableRowGroupFlow {
                                             self.block_flow.base.debug_id());
         debug!("assign_inline_sizes({}): assigning inline_size for flow", "table_rowgroup");
 
         // The position was set to the containing block by the flow's parent.
         let containing_block_inline_size = self.block_flow.base.block_container_inline_size;
         // FIXME: In case of border-collapse: collapse, inline-start_content_edge should be
         // the border width on the inline-start side.
         let inline_start_content_edge = Au::new(0);
+        let inline_end_content_edge = Au::new(0);
         let content_inline_size = containing_block_inline_size;
 
         let inline_size_computer = InternalTable;
         inline_size_computer.compute_used_inline_size(&mut self.block_flow,
                                                       layout_context,
                                                       containing_block_inline_size);
 
         let info = ChildInlineSizeInfo {
             column_computed_inline_sizes: self.column_computed_inline_sizes.as_slice(),
             spacing: self.spacing,
         };
         self.block_flow.propagate_assigned_inline_size_to_children(layout_context,
                                                                    inline_start_content_edge,
+                                                                   inline_end_content_edge,
                                                                    content_inline_size,
                                                                    Some(info));
     }
 
     fn assign_block_size<'a>(&mut self, layout_context: &'a LayoutContext<'a>) {
         debug!("assign_block_size: assigning block_size for table_rowgroup");
         self.block_flow.assign_block_size_for_table_like_flow(layout_context,
                                                               self.spacing.vertical)
--- a/servo/components/layout/table_wrapper.rs
+++ b/servo/components/layout/table_wrapper.rs
@@ -288,16 +288,22 @@ impl Flow for TableWrapperFlow {
                 self.calculate_table_column_sizes_for_automatic_layout(
                     intermediate_column_inline_sizes.as_mut_slice())
             }
         }
 
         let inline_start_content_edge = self.block_flow.fragment.border_box.start.i;
         let content_inline_size = self.block_flow.fragment.border_box.size.inline;
 
+        // FIXME (mbrubeck): Test mixed RTL/LTR table layout, make sure this is right.
+        let inline_end_content_edge = self.block_flow.base.block_container_inline_size -
+                                      self.block_flow.fragment.margin.inline_end -
+                                      content_inline_size -
+                                      inline_start_content_edge;
+
         // In case of fixed layout, column inline-sizes are calculated in table flow.
         let assigned_column_inline_sizes = match self.table_layout {
             TableLayout::Fixed => None,
             TableLayout::Auto => {
                 Some(intermediate_column_inline_sizes.iter().map(|sizes| {
                     ColumnComputedInlineSize {
                         size: sizes.size,
                     }
@@ -305,27 +311,29 @@ impl Flow for TableWrapperFlow {
             }
         };
 
         match assigned_column_inline_sizes {
             None => {
                 self.block_flow.propagate_assigned_inline_size_to_children(
                     layout_context,
                     inline_start_content_edge,
+                    inline_end_content_edge,
                     content_inline_size,
                     None)
             }
             Some(ref assigned_column_inline_sizes) => {
                 let info = ChildInlineSizeInfo {
                     column_computed_inline_sizes: assigned_column_inline_sizes.as_slice(),
                     spacing: self.block_flow.fragment.style().get_inheritedtable().border_spacing,
                 };
                 self.block_flow
                     .propagate_assigned_inline_size_to_children(layout_context,
                                                                 inline_start_content_edge,
+                                                                inline_end_content_edge,
                                                                 content_inline_size,
                                                                 Some(info));
             }
         }
 
     }
 
     fn assign_block_size<'a>(&mut self, ctx: &'a LayoutContext<'a>) {