Auto merge of #7498 - pcwalton:inline-absolute-containing-blocks, r=mbrubeck

layout: Implement partial support for inline absolute containing blocks.

Improves the position of the down arrows on google.com SERPs.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7498)
<!-- Reviewable:end -->

components/layout/construct.rs

@@ -294,14 +294,16 @@ impl<'a> FlowConstructor<'a> {
     }
 
     #[inline]
-    fn set_flow_construction_result(&self, node: &ThreadSafeLayoutNode, result: ConstructionResult) {
+    fn set_flow_construction_result(&self,
-        match result {
+                                    node: &ThreadSafeLayoutNode,
-            ConstructionResult::None => {
+                                    result: ConstructionResult) {
-                let mut layout_data_ref = node.mutate_layout_data();
+        if let ConstructionResult::None = result {
-                let layout_data = layout_data_ref.as_mut().expect("no layout data");
+            let mut layout_data_ref = node.mutate_layout_data();
-                layout_data.remove_compositor_layers(self.layout_context.shared.constellation_chan.clone());
+            let layout_data = layout_data_ref.as_mut().expect("no layout data");
-            }
+            layout_data.remove_compositor_layers(self.layout_context
-            _ => {}
+                                                     .shared
+                                                     .constellation_chan
+                                                     .clone());
         }
 
         node.set_flow_construction_result(result);
@@ -467,8 +469,8 @@ impl<'a> FlowConstructor<'a> {
 
         // Set up absolute descendants as necessary.
         //
-        // TODO(pcwalton): The inline flow itself may need to become the containing block for
+        // The inline flow itself may need to become the containing block for absolute descendants
-        // absolute descendants in order to handle cases like:
+        // in order to handle cases like:
         //
         //      <div>
         //          <span style="position: relative">
@@ -477,6 +479,7 @@ impl<'a> FlowConstructor<'a> {
         //      </div>
         //
         // See the comment above `flow::AbsoluteDescendantInfo` for more information.
+        inline_flow_ref.take_applicable_absolute_descendants(&mut fragments.absolute_descendants);
         absolute_descendants.push_descendants(fragments.absolute_descendants);
 
         {
@@ -878,6 +881,15 @@ impl<'a> FlowConstructor<'a> {
         if opt_inline_block_splits.len() > 0 || !fragment_accumulator.fragments.is_empty()
                 || abs_descendants.len() > 0 {
             fragment_accumulator.fragments.absolute_descendants.push_descendants(abs_descendants);
+
+            // If the node is positioned, then it's the containing block for all absolutely-
+            // positioned descendants.
+            if node.style().get_box().position != position::T::static_ {
+                fragment_accumulator.fragments
+                                    .absolute_descendants
+                                    .mark_as_having_reached_containing_block();
+            }
+
             let construction_item = ConstructionItem::InlineFragments(
                     InlineFragmentsConstructionResult {
                 splits: opt_inline_block_splits,

components/layout/flow.rs

@@ -510,6 +510,17 @@ pub trait MutableOwnedFlowUtils {
     ///
     /// Set this flow as the Containing Block for all the absolute descendants.
     fn set_absolute_descendants(&mut self, abs_descendants: AbsoluteDescendants);
+
+    /// Sets the flow as the containing block for all absolute descendants that have been marked
+    /// as having reached their containing block. This is needed in order to handle cases like:
+    ///
+    ///     <div>
+    ///         <span style="position: relative">
+    ///             <span style="position: absolute; ..."></span>
+    ///         </span>
+    ///     </div>
+    fn take_applicable_absolute_descendants(&mut self,
+                                            absolute_descendants: &mut AbsoluteDescendants);
 }
 
 #[derive(RustcEncodable, PartialEq, Debug)]
@@ -731,6 +742,7 @@ impl AbsoluteDescendants {
     pub fn push(&mut self, given_descendant: FlowRef) {
         self.descendant_links.push(AbsoluteDescendantInfo {
             flow: given_descendant,
+            has_reached_containing_block: false,
         });
     }
 
@@ -749,29 +761,38 @@ impl AbsoluteDescendants {
             iter: self.descendant_links.iter_mut(),
         }
     }
+
+    /// Mark these descendants as having reached their containing block.
+    pub fn mark_as_having_reached_containing_block(&mut self) {
+        for descendant_info in self.descendant_links.iter_mut() {
+            descendant_info.has_reached_containing_block = true
+        }
+    }
 }
 
-/// TODO(pcwalton): This structure is going to need a flag to record whether the absolute
+/// Information about each absolutely-positioned descendant of the given flow.
-/// descendants have reached their containing block yet. The reason is so that we can handle cases
-/// like the following:
-///
-///     <div>
-///         <span id=a style="position: absolute; ...">foo</span>
-///         <span style="position: relative">
-///             <span id=b style="position: absolute; ...">bar</span>
-///         </span>
-///     </div>
-///
-/// When we go to create the `InlineFlow` for the outer `div`, our absolute descendants will
-/// be `a` and `b`. At this point, we need a way to distinguish between the two, because the
-/// containing block for `a` will be different from the containing block for `b`. Specifically,
-/// the latter's containing block is the inline flow itself, while the former's containing
-/// block is going to be some parent of the outer `div`. Hence we need this flag as a way to
-/// distinguish the two; it will be false for `a` and true for `b`.
 #[derive(Clone)]
 pub struct AbsoluteDescendantInfo {
     /// The absolute descendant flow in question.
     flow: FlowRef,
+
+    /// Whether the absolute descendant has reached its containing block. This exists so that we
+    /// can handle cases like the following:
+    ///
+    ///     <div>
+    ///         <span id=a style="position: absolute; ...">foo</span>
+    ///         <span style="position: relative">
+    ///             <span id=b style="position: absolute; ...">bar</span>
+    ///         </span>
+    ///     </div>
+    ///
+    /// When we go to create the `InlineFlow` for the outer `div`, our absolute descendants will
+    /// be `a` and `b`. At this point, we need a way to distinguish between the two, because the
+    /// containing block for `a` will be different from the containing block for `b`. Specifically,
+    /// the latter's containing block is the inline flow itself, while the former's containing
+    /// block is going to be some parent of the outer `div`. Hence we need this flag as a way to
+    /// distinguish the two; it will be false for `a` and true for `b`.
+    has_reached_containing_block: bool,
 }
 
 pub struct AbsoluteDescendantIter<'a> {
@@ -1414,6 +1435,36 @@ impl MutableOwnedFlowUtils for FlowRef {
         let this = self.clone();
         let base = mut_base(flow_ref::deref_mut(self));
         base.abs_descendants = abs_descendants;
+        for descendant_link in base.abs_descendants.descendant_links.iter_mut() {
+            debug_assert!(!descendant_link.has_reached_containing_block);
+            let descendant_base = mut_base(flow_ref::deref_mut(&mut descendant_link.flow));
+            descendant_base.absolute_cb.set(this.clone());
+        }
+    }
+
+    /// Sets the flow as the containing block for all absolute descendants that have been marked
+    /// as having reached their containing block. This is needed in order to handle cases like:
+    ///
+    ///     <div>
+    ///         <span style="position: relative">
+    ///             <span style="position: absolute; ..."></span>
+    ///         </span>
+    ///     </div>
+    fn take_applicable_absolute_descendants(&mut self,
+                                            absolute_descendants: &mut AbsoluteDescendants) {
+        let mut applicable_absolute_descendants = AbsoluteDescendants::new();
+        for absolute_descendant in absolute_descendants.descendant_links.iter() {
+            if absolute_descendant.has_reached_containing_block {
+                applicable_absolute_descendants.push(absolute_descendant.flow.clone());
+            }
+        }
+        absolute_descendants.descendant_links.retain(|descendant| {
+            !descendant.has_reached_containing_block
+        });
+
+        let this = self.clone();
+        let base = mut_base(flow_ref::deref_mut(self));
+        base.abs_descendants = applicable_absolute_descendants;
         for descendant_link in base.abs_descendants.iter() {
             let descendant_base = mut_base(descendant_link);
             descendant_base.absolute_cb.set(this.clone());

components/layout/fragment.rs

@@ -2272,6 +2272,11 @@ impl Fragment {
         }
         false
     }
+
+    /// Returns true if this node is absolutely positioned.
+    pub fn is_absolutely_positioned(&self) -> bool {
+        self.style.get_box().position == position::T::absolute
+    }
 }
 
 impl fmt::Debug for Fragment {

components/layout/inline.rs

@@ -1277,7 +1277,7 @@ impl InlineFlow {
     }
 
     fn containing_block_range_for_flow(&self, opaque_flow: OpaqueFlow) -> Range<FragmentIndex> {
-        let index = FragmentIndex(self.fragments.fragments.iter().position(|fragment| {
+        match self.fragments.fragments.iter().position(|fragment| {
             match fragment.specific {
                 SpecificFragmentInfo::InlineAbsolute(ref inline_absolute) => {
                     OpaqueFlow::from_flow(&*inline_absolute.flow_ref) == opaque_flow
@@ -1288,9 +1288,19 @@ impl InlineFlow {
                 }
                 _ => false,
             }
-        }).expect("containing_block_range_for_flow(): couldn't find inline absolute fragment!")
+        }) {
-                                 as isize);
+            Some(index) => {
-        self.containing_block_range_for_flow_surrounding_fragment_at_index(index)
+                let index = FragmentIndex(index as isize);
+                self.containing_block_range_for_flow_surrounding_fragment_at_index(index)
+            }
+            None => {
+                // FIXME(pcwalton): This is quite wrong. We should only return the range
+                // surrounding the inline fragments that constitute the containing block. But this
+                // suffices to get Google looking right.
+                Range::new(FragmentIndex(0),
+                           FragmentIndex(self.fragments.fragments.len() as isize))
+            }
+        }
     }
 }
 
@@ -1766,9 +1776,7 @@ impl Flow for InlineFlow {
     }
 
     fn contains_positioned_fragments(&self) -> bool {
-        self.fragments.fragments.iter().any(|fragment| {
+        self.fragments.fragments.iter().any(|fragment| fragment.is_positioned())
-            fragment.style.get_box().position != position::T::static_
-        })
     }
 
     fn contains_relatively_positioned_fragments(&self) -> bool {
@@ -1781,10 +1789,13 @@ impl Flow for InlineFlow {
         let mut containing_block_size = LogicalSize::new(self.base.writing_mode, Au(0), Au(0));
         for index in self.containing_block_range_for_flow(for_flow).each_index() {
             let fragment = &self.fragments.fragments[index.get() as usize];
+            if fragment.is_absolutely_positioned() {
+                continue
+            }
             containing_block_size.inline = containing_block_size.inline +
                 fragment.border_box.size.inline;
             containing_block_size.block = max(containing_block_size.block,
-                                              fragment.border_box.size.block)
+                                              fragment.border_box.size.block);
         }
         containing_block_size
     }

tests/ref/basic.list

@@ -313,6 +313,7 @@ flaky_cpu == linebreak_simple_a.html linebreak_simple_b.html
 == rtl_table_a.html rtl_table_ref.html
 == servo_center_a.html servo_center_ref.html
 == setattribute_id_restyle_a.html setattribute_id_restyle_b.html
+== simple_inline_absolute_containing_block_a.html simple_inline_absolute_containing_block_ref.html
 == stacking_context_overflow_a.html stacking_context_overflow_ref.html
 == stacking_context_overflow_relative_outline_a.html stacking_context_overflow_relative_outline_ref.html
 == style_is_in_doc.html style_is_in_doc_ref.html

tests/ref/simple_inline_absolute_containing_block_a.html

@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<style>
+main {
+    position: relative;
+    padding: 0 16px;
+    display: inline;
+}
+section {
+    position: absolute;
+    top: 25%;
+    bottom: 25%;
+    left: 0;
+    right: 0;
+    background: red;
+}
+#coveritup {
+    position: relative;
+    background: white;
+    top: -20px;
+    display: inline;
+    color: white;
+}
+</style>
+<div>There should be no red.<main><section></section></main></div>
+<div>There should be no red.<div id=coveritup>XXXXX</div></div>
+

tests/ref/simple_inline_absolute_containing_block_ref.html

@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<div>There should be no red.</div>
+<div>There should be no red.</div>
+