layout: Implement inline margins.

Improves the Google SERPs.

We mark `html/rendering/replaced-elements/images/space.html` as failing.
This test tested whether `<img hspace>` and inline margins do the same
thing. Since this was trivially the case before (since we implemented
neither) and now is not, this test now fails.

components/layout/construct.rs

@@ -334,7 +334,9 @@ impl<'a> FlowConstructor<'a> {
         if child.is_table() {
             let fragment = Fragment::new(child_node, SpecificFragmentInfo::TableWrapper);
             let mut new_child =
-                FlowRef::new(box TableWrapperFlow::from_node_and_fragment(child_node, fragment, None));
+                FlowRef::new(box TableWrapperFlow::from_node_and_fragment(child_node,
+                                                                          fragment,
+                                                                          None));
             new_child.add_new_child(child.clone());
             child.finish();
             *child = new_child
@@ -376,8 +378,8 @@ impl<'a> FlowConstructor<'a> {
 
         // Build a list of all the inline-block fragments before fragments is moved.
         let mut inline_block_flows = vec!();
-        for f in fragments.iter() {
+        for fragment in fragments.iter() {
-            match f.specific {
+            match fragment.specific {
                 SpecificFragmentInfo::InlineBlock(ref info) => {
                     inline_block_flows.push(info.flow_ref.clone())
                 }
@@ -511,13 +513,15 @@ impl<'a> FlowConstructor<'a> {
                 inline_fragment_accumulator.push_all(successor_fragments);
                 abs_descendants.push_descendants(kid_abs_descendants);
             }
-            ConstructionResult::ConstructionItem(ConstructionItem::Whitespace(whitespace_node,
+            ConstructionResult::ConstructionItem(ConstructionItem::Whitespace(
-                                                                          whitespace_style,
+                    whitespace_node,
-                                                                          whitespace_damage)) => {
+                    mut whitespace_style,
+                    whitespace_damage)) => {
                 // Add whitespace results. They will be stripped out later on when
                 // between block elements, and retained when between inline elements.
                 let fragment_info = SpecificFragmentInfo::UnscannedText(
                     UnscannedTextFragmentInfo::from_text(" ".to_owned()));
+                properties::modify_style_for_replaced_content(&mut whitespace_style);
                 let fragment = Fragment::from_opaque_node_and_style(whitespace_node,
                                                                     whitespace_style,
                                                                     whitespace_damage,
@@ -729,11 +733,12 @@ impl<'a> FlowConstructor<'a> {
                 }
                 ConstructionResult::ConstructionItem(ConstructionItem::Whitespace(
                         whitespace_node,
-                        whitespace_style,
+                        mut whitespace_style,
                         whitespace_damage)) => {
                     // Instantiate the whitespace fragment.
                     let fragment_info = SpecificFragmentInfo::UnscannedText(
                         UnscannedTextFragmentInfo::from_text(" ".to_owned()));
+                    properties::modify_style_for_replaced_content(&mut whitespace_style);
                     let fragment = Fragment::from_opaque_node_and_style(whitespace_node,
                                                                         whitespace_style,
                                                                         whitespace_damage,

components/layout/fragment.rs

@@ -21,7 +21,6 @@ use text;
 use opaque_node::OpaqueNodeMethods;
 use wrapper::{TLayoutNode, ThreadSafeLayoutNode};
 
-use geom::num::Zero;
 use geom::{Point2D, Rect, Size2D};
 use gfx::display_list::{BLUR_INFLATION_FACTOR, OpaqueNode};
 use gfx::text::glyph::CharIndex;
@@ -42,7 +41,7 @@ use style::computed_values::content::ContentItem;
 use style::computed_values::{border_collapse, clear, mix_blend_mode, overflow_wrap, position};
 use style::computed_values::{text_align, text_decoration, white_space, word_break};
 use style::node::{TElement, TNode};
-use style::properties::{ComputedValues, cascade_anonymous, make_border};
+use style::properties::{self, ComputedValues, cascade_anonymous};
 use style::values::computed::{LengthOrPercentage, LengthOrPercentageOrAuto};
 use style::values::computed::{LengthOrPercentageOrNone};
 use text::TextRunScanner;
@@ -855,18 +854,7 @@ impl Fragment {
             self.inline_context = Some(InlineFragmentContext::new());
         }
         if !first_frag || !last_frag {
-            // Set the border width to zero and the border style to none on
+            properties::modify_style_for_inline_sides(&mut node_info.style, first_frag, last_frag)
-            // border sides that are not the outermost for a node container.
-            // Because with multiple inline fragments they don't have interior
-            // borders separating each other.
-            let mut border_width = node_info.style.logical_border_width();
-            if !last_frag {
-                border_width.set_right(node_info.style.writing_mode, Zero::zero());
-            }
-            if !first_frag {
-                border_width.set_left(node_info.style.writing_mode, Zero::zero());
-            }
-            node_info.style = Arc::new(make_border(&*node_info.style, border_width))
         };
         self.inline_context.as_mut().unwrap().nodes.push(node_info);
     }
@@ -1018,21 +1006,36 @@ impl Fragment {
     /// (for example, via constraint solving for blocks).
     pub fn compute_inline_direction_margins(&mut self, containing_block_inline_size: Au) {
         match self.specific {
+            SpecificFragmentInfo::InlineBlock(_) |
             SpecificFragmentInfo::Table |
             SpecificFragmentInfo::TableCell |
             SpecificFragmentInfo::TableRow |
             SpecificFragmentInfo::TableColumn(_) => {
                 self.margin.inline_start = Au(0);
-                self.margin.inline_end = Au(0)
+                self.margin.inline_end = Au(0);
+                return
             }
-            _ => {
+            _ => {}
-                let margin = self.style().logical_margin();
+        }
-                self.margin.inline_start =
+
-                    MaybeAuto::from_style(margin.inline_start, containing_block_inline_size)
+
-                    .specified_or_zero();
+        let margin = self.style().logical_margin();
-                self.margin.inline_end =
+        self.margin.inline_start =
-                    MaybeAuto::from_style(margin.inline_end, containing_block_inline_size)
+            MaybeAuto::from_style(margin.inline_start,
-                    .specified_or_zero();
+                                  containing_block_inline_size).specified_or_zero();
+        self.margin.inline_end =
+            MaybeAuto::from_style(margin.inline_end,
+                                  containing_block_inline_size).specified_or_zero();
+
+        if let Some(ref inline_context) = self.inline_context {
+            for node in inline_context.nodes.iter() {
+                let margin = node.style.logical_margin();
+                self.margin.inline_start = self.margin.inline_start +
+                    MaybeAuto::from_style(margin.inline_start,
+                                          containing_block_inline_size).specified_or_zero();
+                self.margin.inline_end = self.margin.inline_end +
+                    MaybeAuto::from_style(margin.inline_end,
+                                          containing_block_inline_size).specified_or_zero();
             }
         }
     }
@@ -2027,6 +2030,11 @@ impl Fragment {
     pub fn inline_styles<'a>(&'a self) -> InlineStyleIterator<'a> {
         InlineStyleIterator::new(self)
     }
+
+    /// Returns the inline-size of this fragment's margin box.
+    pub fn margin_box_inline_size(&self) -> Au {
+        self.border_box.size.inline + self.margin.inline_start_end()
+    }
 }
 
 impl fmt::Debug for Fragment {

components/layout/inline.rs

@@ -378,13 +378,15 @@ impl LineBreaker {
             let mut range = &mut scanned_text_fragment_info.range;
             strip_trailing_whitespace_if_necessary(&**scanned_text_fragment_info.run, range);
 
-            let old_fragment_inline_size = fragment.border_box.size.inline;
+            let old_fragment_inline_size = fragment.border_box.size.inline +
+                fragment.margin.inline_start_end();
             scanned_text_fragment_info.content_size.inline =
                 scanned_text_fragment_info.run.metrics_for_range(range).advance_width;
             fragment.border_box.size.inline = scanned_text_fragment_info.content_size.inline +
                 fragment.border_padding.inline_start_end();
             self.pending_line.bounds.size.inline = self.pending_line.bounds.size.inline -
-                (old_fragment_inline_size - fragment.border_box.size.inline)
+                (old_fragment_inline_size - (fragment.border_box.size.inline +
+                                             fragment.margin.inline_start_end()))
         }
     }
 
@@ -420,7 +422,7 @@ impl LineBreaker {
         let placement_inline_size = if first_fragment.can_split() {
             Au(0)
         } else {
-            first_fragment.border_box.size.inline + self.indentation_for_pending_fragment()
+            first_fragment.margin_box_inline_size() + self.indentation_for_pending_fragment()
         };
 
         // Try to place the fragment between floats.
@@ -434,9 +436,9 @@ impl LineBreaker {
         });
 
         // Simple case: if the fragment fits, then we can stop here.
-        if line_bounds.size.inline > first_fragment.border_box.size.inline {
+        if line_bounds.size.inline > first_fragment.margin_box_inline_size() {
             debug!("LineBreaker: fragment fits on line {}", self.lines.len());
-            return (line_bounds, first_fragment.border_box.size.inline);
+            return (line_bounds, first_fragment.margin_box_inline_size());
         }
 
         // If not, but we can't split the fragment, then we'll place the line here and it will
@@ -445,7 +447,7 @@ impl LineBreaker {
             debug!("LineBreaker: line doesn't fit, but is unsplittable");
         }
 
-        (line_bounds, first_fragment.border_box.size.inline)
+        (line_bounds, first_fragment.margin_box_inline_size())
     }
 
     /// Performs float collision avoidance. This is called when adding a fragment is going to
@@ -545,7 +547,7 @@ impl LineBreaker {
         // it doesn't fit.
         let indentation = self.indentation_for_pending_fragment();
         let new_inline_size = self.pending_line.bounds.size.inline +
-            fragment.border_box.size.inline + indentation;
+            fragment.margin_box_inline_size() + indentation;
         if new_inline_size <= green_zone.inline {
             debug!("LineBreaker: fragment fits without splitting");
             self.push_fragment_to_line(layout_context, fragment, line_flush_mode);
@@ -632,7 +634,7 @@ impl LineBreaker {
                fragment.style().get_box().overflow_x) {
             (text_overflow::T::clip, _) | (_, overflow_x::T::visible) => {}
             (text_overflow::T::ellipsis, _) => {
-                need_ellipsis = fragment.border_box.size.inline > available_inline_size;
+                need_ellipsis = fragment.margin_box_inline_size() > available_inline_size;
             }
         }
 
@@ -642,7 +644,7 @@ impl LineBreaker {
             let ellipsis = fragment.transform_into_ellipsis(layout_context);
             if let Some(truncation_info) =
                     fragment.truncate_to_inline_size(available_inline_size -
-                                                     ellipsis.border_box.size.inline) {
+                                                     ellipsis.margin_box_inline_size()) {
                 let fragment = fragment.transform_with_split_info(&truncation_info.split,
                                                                   truncation_info.text_run);
                 self.push_fragment_to_line_ignoring_text_overflow(fragment, layout_context);
@@ -663,7 +665,7 @@ impl LineBreaker {
         let indentation = self.indentation_for_pending_fragment();
         self.pending_line.range.extend_by(FragmentIndex(1));
         self.pending_line.bounds.size.inline = self.pending_line.bounds.size.inline +
-            fragment.border_box.size.inline +
+            fragment.margin_box_inline_size() +
             indentation;
         self.pending_line.inline_metrics =
             self.new_inline_metrics_for_line(&fragment, layout_context);
@@ -928,14 +930,16 @@ impl InlineFlow {
 
         for fragment_index in line.range.each_index() {
             let fragment = fragments.get_mut(fragment_index.to_usize());
-            let size = fragment.border_box.size;
+            inline_start_position_for_fragment = inline_start_position_for_fragment +
+                fragment.margin.inline_start;
             fragment.border_box = LogicalRect::new(fragment.style.writing_mode,
                                                    inline_start_position_for_fragment,
                                                    fragment.border_box.start.b,
-                                                   size.inline,
+                                                   fragment.border_box.size.inline,
-                                                   size.block);
+                                                   fragment.border_box.size.block);
             fragment.update_late_computed_inline_position_if_necessary();
-            inline_start_position_for_fragment = inline_start_position_for_fragment + size.inline;
+            inline_start_position_for_fragment = inline_start_position_for_fragment +
+                fragment.border_box.size.inline + fragment.margin.inline_end;
         }
     }
 

components/style/properties.mako.rs

@@ -15,12 +15,11 @@ use std::hash::{Hash, Hasher};
 use std::sync::Arc;
 
 use util::fnv::FnvHasher;
-use util::logical_geometry::{WritingMode, LogicalMargin};
+use util::logical_geometry::{LogicalMargin, PhysicalSide, WritingMode};
 use util::geometry::Au;
 use url::Url;
 use cssparser::{Parser, Color, RGBA, AtRuleParser, DeclarationParser,
                 DeclarationListParser, parse_important, ToCss};
-use geom::num::Zero;
 use geom::SideOffsets2D;
 use geom::size::Size2D;
 
@@ -5723,21 +5722,78 @@ pub fn modify_style_for_replaced_content(style: &mut Arc<ComputedValues>) {
         style.box_.make_unique().vertical_align =
             longhands::vertical_align::computed_value::T::baseline
     }
+
+    // Reset margins.
+    if style.margin.margin_top != computed::LengthOrPercentageOrAuto::Length(Au(0)) ||
+            style.margin.margin_left != computed::LengthOrPercentageOrAuto::Length(Au(0)) ||
+            style.margin.margin_bottom != computed::LengthOrPercentageOrAuto::Length(Au(0)) ||
+            style.margin.margin_right != computed::LengthOrPercentageOrAuto::Length(Au(0)) {
+        let mut style = style.make_unique();
+        let margin = style.margin.make_unique();
+        margin.margin_top = computed::LengthOrPercentageOrAuto::Length(Au(0));
+        margin.margin_left = computed::LengthOrPercentageOrAuto::Length(Au(0));
+        margin.margin_bottom = computed::LengthOrPercentageOrAuto::Length(Au(0));
+        margin.margin_right = computed::LengthOrPercentageOrAuto::Length(Au(0));
+    }
 }
 
-/// Sets `border_${side}_width` to the passed in values.
+/// Adjusts borders, padding, and margins as appropriate to account for a fragment's status as the
-/// If `border_${side}_width` == 0 also sets `border_${side}_style` = none.
+/// first or last fragment within the range of an element.
+///
+/// Specifically, this function sets border/padding/margin widths to zero on the sides for which
+/// the fragment is not outermost.
 #[inline]
-pub fn make_border(style: &ComputedValues, border_width: LogicalMargin<Au>) -> ComputedValues {
+pub fn modify_style_for_inline_sides(style: &mut Arc<ComputedValues>,
-    let mut style = (*style).clone();
+                                     is_first_fragment_of_element: bool,
-    let physical_border = LogicalMargin::to_physical(&border_width, style.writing_mode);
+                                     is_last_fragment_of_element: bool) {
-    % for side in ["top", "right", "bottom", "left"]:
+    fn modify_side(style: &mut Arc<ComputedValues>, side: PhysicalSide) {
-        style.border.make_unique().border_${side}_width = physical_border.${side};
+        let mut style = style.make_unique();
-        if physical_border.${side} == Zero::zero() {
+        let border = style.border.make_unique();
-            style.border.make_unique().border_${side}_style = BorderStyle::none;
+        match side {
+            PhysicalSide::Left => {
+                border.border_left_width = Au(0);
+                border.border_left_style = BorderStyle::none;
+                style.padding.make_unique().padding_left =
+                    computed::LengthOrPercentage::Length(Au(0));
+                style.margin.make_unique().margin_left =
+                    computed::LengthOrPercentageOrAuto::Length(Au(0))
+            }
+            PhysicalSide::Right => {
+                border.border_right_width = Au(0);
+                border.border_right_style = BorderStyle::none;
+                style.padding.make_unique().padding_right =
+                    computed::LengthOrPercentage::Length(Au(0));
+                style.margin.make_unique().margin_right =
+                    computed::LengthOrPercentageOrAuto::Length(Au(0))
+            }
+            PhysicalSide::Bottom => {
+                border.border_bottom_width = Au(0);
+                border.border_bottom_style = BorderStyle::none;
+                style.padding.make_unique().padding_bottom =
+                    computed::LengthOrPercentage::Length(Au(0));
+                style.margin.make_unique().margin_bottom =
+                    computed::LengthOrPercentageOrAuto::Length(Au(0))
+            }
+            PhysicalSide::Top => {
+                border.border_top_width = Au(0);
+                border.border_top_style = BorderStyle::none;
+                style.padding.make_unique().padding_top =
+                    computed::LengthOrPercentage::Length(Au(0));
+                style.margin.make_unique().margin_top =
+                    computed::LengthOrPercentageOrAuto::Length(Au(0))
+            }
         }
-    % endfor
+    }
-    style
+
+    if !is_first_fragment_of_element {
+        let side = style.writing_mode.inline_start_physical_side();
+        modify_side(style, side)
+    }
+
+    if !is_last_fragment_of_element {
+        let side = style.writing_mode.inline_end_physical_side();
+        modify_side(style, side)
+    }
 }
 
 pub fn is_supported_property(property: &str) -> bool {

tests/ref/basic.list

@@ -151,6 +151,8 @@ flaky_cpu == append_style_a.html append_style_b.html
 # inline_border_a.html inline_border_b.html
 == inline_element_border_a.html inline_element_border_ref.html
 == inline_hypothetical_box_a.html inline_hypothetical_box_ref.html
+== inline_margin_multiple_fragments_a.html inline_margin_multiple_fragments_ref.html
+== inline_margins_a.html inline_margins_ref.html
 == inline_padding_a.html inline_padding_b.html
 # inline_text_align_a.html inline_text_align_b.html
 == inline_whitespace_a.html inline_whitespace_ref.html

tests/ref/inline_margin_multiple_fragments_a.html

@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+span {
+    margin-left: 128px;
+    margin-right: 128px;
+}
+</style>
+</head>
+<body>
+<span><img src=rust.png><img src=rust.png><img src=rust.png></span>
+</body>
+</html>
+

tests/ref/inline_margin_multiple_fragments_ref.html

@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+#first {
+    margin-left: 128px;
+}
+#last {
+    margin-right: 128px;
+}
+</style>
+</head>
+<body>
+<span id=first><img src=rust.png></span><img src=rust.png><span id=last><img src=rust.png></span>
+</body>
+</html>
+
+

tests/ref/inline_margins_a.html

@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+<link rel="stylesheet" type="text/css" href="css/ahem.css">
+<style>
+span {
+    margin-left: 100px;
+    margin-right: 100px;
+}
+</style>
+</head>
+<body>x<span>x</span>x</body>
+</html>
+

tests/ref/inline_margins_ref.html

@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<html>
+<head>
+<link rel="stylesheet" type="text/css" href="css/ahem.css">
+</head>
+<body>x x x</body>
+</html>
+

tests/wpt/metadata/html/rendering/replaced-elements/images/space.html.ini

@@ -0,0 +1,3 @@
+[space.html]
+  type: reftest
+  expected: FAIL