Auto merge of #6903 - glennw:fix-height-again, r=pcwalton

Fix percentage height calculation, absolute containing block height calculations.

It's not possible to correctly determine during the css cascade whether the container height
is explicitly specified. Additionally, the spec https://drafts.csswg.org/css2/visudet.html#the-height-property says this should affect the *used* height, rather than the computed height.

This significantly improves the layout in #6643.

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

components/layout/block.rs

@@ -699,7 +699,7 @@ impl BlockFlow {
     /// Right now, this only gets the containing block size for absolutely positioned elements.
     /// Note: We assume this is called in a top-down traversal, so it is ok to reference the CB.
     #[inline]
-    pub fn containing_block_size(&mut self, viewport_size: &Size2D<Au>, descendant: OpaqueFlow)
+    pub fn containing_block_size(&self, viewport_size: &Size2D<Au>, descendant: OpaqueFlow)
                                  -> LogicalSize<Au> {
         debug_assert!(self.base.flags.contains(IS_ABSOLUTELY_POSITIONED));
         if self.is_fixed() {
@@ -929,6 +929,7 @@ impl BlockFlow {
             let (collapsible_margins, delta) =
                 margin_collapse_info.finish_and_compute_collapsible_margins(
                 &self.fragment,
+                self.base.block_container_explicit_block_size,
                 can_collapse_block_end_margin_with_kids);
             self.base.collapsible_margins = collapsible_margins;
             translate_including_floats(&mut cur_b, delta, &mut floats);
@@ -1095,14 +1096,69 @@ impl BlockFlow {
                                                           self.base.position.size);
     }
 
+    pub fn explicit_block_containing_size(&self, layout_context: &LayoutContext) -> Option<Au> {
+        if self.is_root() || self.is_fixed() {
+            let screen_size = LogicalSize::from_physical(self.fragment.style.writing_mode,
+                                                         layout_context.shared.screen_size);
+            Some(screen_size.block)
+        } else if self.base.flags.contains(IS_ABSOLUTELY_POSITIONED) {
+            self.base.absolute_cb.explicit_block_containing_size(layout_context)
+        } else {
+            self.base.block_container_explicit_block_size
+        }
+    }
+
+    fn explicit_block_size(&self, containing_block_size: Option<Au>) -> Option<Au> {
+        let content_block_size = self.fragment.style().content_block_size();
+
+        match (content_block_size, containing_block_size) {
+            (LengthOrPercentageOrAuto::Length(length), _) => Some(length),
+            (LengthOrPercentageOrAuto::Percentage(percent), Some(container_size)) => {
+                Some(container_size.scale_by(percent))
+            }
+            (LengthOrPercentageOrAuto::Percentage(_), None) => {
+                None
+            }
+            (LengthOrPercentageOrAuto::Auto, None) => {
+                None
+            }
+            (LengthOrPercentageOrAuto::Auto, Some(container_size)) => {
+                let (block_start, block_end) = {
+                    let position = self.fragment.style().logical_position();
+                    (MaybeAuto::from_style(position.block_start, container_size),
+                     MaybeAuto::from_style(position.block_end, container_size))
+                };
+
+                match (block_start, block_end) {
+                    (MaybeAuto::Specified(block_start), MaybeAuto::Specified(block_end)) => {
+                        let available_block_size = container_size - self.fragment.border_padding.block_start_end();
+
+                        // Non-auto margin-block-start and margin-block-end values have already been
+                        // calculated during assign-inline-size.
+                        let margin = self.fragment.style().logical_margin();
+                        let margin_block_start = match margin.block_start {
+                            LengthOrPercentageOrAuto::Auto => MaybeAuto::Auto,
+                            _ => MaybeAuto::Specified(self.fragment.margin.block_start)
+                        };
+                        let margin_block_end = match margin.block_end {
+                            LengthOrPercentageOrAuto::Auto => MaybeAuto::Auto,
+                            _ => MaybeAuto::Specified(self.fragment.margin.block_end)
+                        };
+
+                        let margin_block_start = margin_block_start.specified_or_zero();
+                        let margin_block_end = margin_block_end.specified_or_zero();
+                        let sum = block_start + block_end + margin_block_start + margin_block_end;
+                        Some(available_block_size - sum)
+                    }
+
+                    (_, _) => {
+                        None
+                    }
+                }
+            }
+        }
+    }
 
-    /// Calculate and set the block-size, offsets, etc. for absolutely positioned flow.
-    ///
-    /// The layout for its in-flow children has been done during normal layout.
-    /// This is just the calculation of:
-    /// + block-size for the flow
-    /// + position in the block direction of the flow with respect to its Containing Block.
-    /// + block-size, vertical margins, and y-coordinate for the flow's box.
     fn calculate_absolute_block_size_and_margins(&mut self, layout_context: &LayoutContext) {
         let opaque_self = OpaqueFlow::from_flow(self);
         let containing_block_block_size =
@@ -1140,7 +1196,7 @@ impl BlockFlow {
                 // Calculate used value of block-size just like we do for inline replaced elements.
                 // TODO: Pass in the containing block block-size when Fragment's
                 // assign-block-size can handle it correctly.
-                self.fragment.assign_replaced_block_size_if_necessary(containing_block_block_size);
+                self.fragment.assign_replaced_block_size_if_necessary(Some(containing_block_block_size));
                 // TODO: Right now, this content block-size value includes the
                 // margin because of erroneous block-size calculation in fragment.
                 // Check this when that has been fixed.
@@ -1248,28 +1304,13 @@ impl BlockFlow {
             inline_size_of_preceding_right_floats = self.inline_size_of_preceding_right_floats;
         }
 
+        let opaque_self = OpaqueFlow::from_flow(self);
+
         // Calculate non-auto block size to pass to children.
-        let content_block_size = self.fragment.style().content_block_size();
+        let parent_container_size = self.explicit_block_containing_size(layout_context);
-
+        let explicit_content_size = self.explicit_block_size(parent_container_size);
-        let parent_container_size = if self.is_root() {
-            let screen_size = LogicalSize::from_physical(self.fragment.style.writing_mode,
-                                                         layout_context.shared.screen_size);
-            Some(screen_size.block)
-        } else {
-            self.base.block_container_explicit_block_size
-        };
-
-        let explicit_content_size = match (content_block_size, parent_container_size) {
-            (LengthOrPercentageOrAuto::Percentage(percent), Some(container_size)) => {
-                Some(container_size.scale_by(percent))
-            }
-            (LengthOrPercentageOrAuto::Percentage(_), None) |
-            (LengthOrPercentageOrAuto::Auto, _) => None,
-            (LengthOrPercentageOrAuto::Length(length), _) => Some(length),
-        };
 
         // Calculate containing block inline size.
-        let opaque_self = OpaqueFlow::from_flow(self);
         let containing_block_size = if flags.contains(IS_ABSOLUTELY_POSITIONED) {
             self.containing_block_size(&layout_context.shared.screen_size, opaque_self).inline
         } else {
@@ -1286,8 +1327,10 @@ impl BlockFlow {
         while let Some((i, kid)) = iterator.next() {
             {
                 let kid_base = flow::mut_base(kid);
+                if !kid_base.flags.contains(IS_ABSOLUTELY_POSITIONED) {
                     kid_base.block_container_explicit_block_size = explicit_content_size;
                 }
+            }
 
             // Determine float impaction, and update the inline size speculations if necessary.
             if flow::base(kid).flags.contains(CLEARS_LEFT) {
@@ -1675,7 +1718,7 @@ impl Flow for BlockFlow {
 
             // Assign block-size for fragment if it is an image fragment.
             let containing_block_block_size =
-                self.base.block_container_explicit_block_size.unwrap_or(Au(0));
+                self.base.block_container_explicit_block_size;
             self.fragment.assign_replaced_block_size_if_necessary(containing_block_block_size);
             if !self.base.flags.contains(IS_ABSOLUTELY_POSITIONED) {
                 self.base.position.size.block = self.fragment.border_box.size.block;

components/layout/flow.rs

@@ -1389,7 +1389,7 @@ impl ContainingBlockLink {
     }
 
     #[inline]
-    pub fn generated_containing_block_size(&mut self, for_flow: OpaqueFlow) -> LogicalSize<Au> {
+    pub fn generated_containing_block_size(&self, for_flow: OpaqueFlow) -> LogicalSize<Au> {
         match self.link {
             None => {
                 panic!("Link to containing block not established; perhaps you forgot to call \
@@ -1398,6 +1398,24 @@ impl ContainingBlockLink {
             Some(ref link) => link.upgrade().unwrap().generated_containing_block_size(for_flow),
         }
     }
+
+    #[inline]
+    pub fn explicit_block_containing_size(&self, layout_context: &LayoutContext) -> Option<Au> {
+        match self.link {
+            None => {
+                panic!("Link to containing block not established; perhaps you forgot to call \
+                        `set_absolute_descendants`?")
+            }
+            Some(ref link) => {
+                let flow = link.upgrade().unwrap();
+                if flow.is_block_like() {
+                    flow.as_immutable_block().explicit_block_containing_size(layout_context)
+                } else {
+                    None
+                }
+            }
+        }
+    }
 }
 
 /// A wrapper for the pointer address of a flow. These pointer addresses may only be compared for

components/layout/fragment.rs

@@ -216,10 +216,10 @@ impl fmt::Debug for SpecificFragmentInfo {
 fn clamp_size(size: Au,
               min_size: LengthOrPercentage,
               max_size: LengthOrPercentageOrNone,
-              container_inline_size: Au)
+              container_size: Au)
               -> Au {
-    let min_size = model::specified(min_size, container_inline_size);
+    let min_size = model::specified(min_size, container_size);
-    let max_size = model::specified_or_none(max_size, container_inline_size);
+    let max_size = model::specified_or_none(max_size, container_size);
 
     max(min_size, match max_size {
         None => size,
@@ -442,11 +442,15 @@ impl ReplacedImageFragmentInfo {
     // `style_length`: inline-size as given in the CSS
     pub fn style_length(style_length: LengthOrPercentageOrAuto,
                         dom_length: Option<Au>,
-                        container_inline_size: Au) -> MaybeAuto {
+                        container_size: Option<Au>) -> MaybeAuto {
-        match (MaybeAuto::from_style(style_length,container_inline_size), dom_length) {
+        match (style_length, dom_length, container_size) {
-            (MaybeAuto::Specified(length), _) => MaybeAuto::Specified(length),
+            (LengthOrPercentageOrAuto::Length(length), _, _) => MaybeAuto::Specified(length),
-            (MaybeAuto::Auto, Some(length)) => MaybeAuto::Specified(length),
+            (LengthOrPercentageOrAuto::Percentage(pc), _, Some(container_size)) => {
-            (MaybeAuto::Auto, None) => MaybeAuto::Auto,
+                MaybeAuto::Specified(container_size.scale_by(pc))
+            }
+            (LengthOrPercentageOrAuto::Percentage(_), _, None) => MaybeAuto::Auto,
+            (LengthOrPercentageOrAuto::Auto, Some(dom_length), _) => MaybeAuto::Specified(dom_length),
+            (LengthOrPercentageOrAuto::Auto, None, _) => MaybeAuto::Auto,
         }
     }
 
@@ -468,7 +472,7 @@ impl ReplacedImageFragmentInfo {
         let inline_size = ReplacedImageFragmentInfo::style_length(
             style_inline_size,
             self.dom_inline_size,
-            container_inline_size);
+            Some(container_inline_size));
 
         let inline_size = match inline_size {
             MaybeAuto::Auto => {
@@ -483,7 +487,7 @@ impl ReplacedImageFragmentInfo {
                     let specified_height = ReplacedImageFragmentInfo::style_length(
                         style_block_size,
                         self.dom_block_size,
-                        Au(0));
+                        None);
                     let specified_height = match specified_height {
                         MaybeAuto::Auto => intrinsic_height,
                         MaybeAuto::Specified(h) => h,
@@ -510,7 +514,7 @@ impl ReplacedImageFragmentInfo {
     pub fn calculate_replaced_block_size(&mut self,
                                          style: &ComputedValues,
                                          noncontent_block_size: Au,
-                                         containing_block_block_size: Au,
+                                         containing_block_block_size: Option<Au>,
                                          fragment_inline_size: Au,
                                          fragment_block_size: Au)
                                          -> Au {
@@ -574,12 +578,12 @@ impl IframeFragmentInfo {
         IframeFragmentInfo::calculate_replaced_size(style.content_inline_size(),
                                                     style.min_inline_size(),
                                                     style.max_inline_size(),
-                                                    containing_size,
+                                                    Some(containing_size),
                                                     Au::from_px(300))
     }
 
     #[inline]
-    pub fn calculate_replaced_block_size(&self, style: &ComputedValues, containing_size: Au)
+    pub fn calculate_replaced_block_size(&self, style: &ComputedValues, containing_size: Option<Au>)
                                          -> Au {
         // Calculate the replaced block size (or default) as per CSS 2.1 § 10.3.2
         IframeFragmentInfo::calculate_replaced_size(style.content_block_size(),
@@ -593,13 +597,16 @@ impl IframeFragmentInfo {
     fn calculate_replaced_size(content_size: LengthOrPercentageOrAuto,
                                style_min_size: LengthOrPercentage,
                                style_max_size: LengthOrPercentageOrNone,
-                               containing_size: Au,
+                               containing_size: Option<Au>,
                                default_size: Au) -> Au {
-        let computed_size = match MaybeAuto::from_style(content_size, containing_size) {
+        let computed_size = match (content_size, containing_size) {
-            MaybeAuto::Specified(length) => length,
+            (LengthOrPercentageOrAuto::Length(length), _) => length,
-            MaybeAuto::Auto => default_size,
+            (LengthOrPercentageOrAuto::Percentage(pc), Some(container_size)) => container_size.scale_by(pc),
+            (LengthOrPercentageOrAuto::Percentage(_), None) => default_size,
+            (LengthOrPercentageOrAuto::Auto, _) => default_size,
         };
 
+        let containing_size = containing_size.unwrap_or(Au(0));
         let size = clamp_size(computed_size,
                               style_min_size,
                               style_max_size,
@@ -1714,7 +1721,7 @@ impl Fragment {
     /// been assigned first.
     ///
     /// Ideally, this should follow CSS 2.1 § 10.6.2.
-    pub fn assign_replaced_block_size_if_necessary(&mut self, containing_block_block_size: Au) {
+    pub fn assign_replaced_block_size_if_necessary(&mut self, containing_block_block_size: Option<Au>) {
         match self.specific {
             SpecificFragmentInfo::Generic |
             SpecificFragmentInfo::GeneratedContent(_) |

components/layout/inline.rs

@@ -1346,7 +1346,7 @@ impl Flow for InlineFlow {
 
         // Assign the block-size and late-computed inline-sizes for the inline fragments.
         let containing_block_block_size =
-            self.base.block_container_explicit_block_size.unwrap_or(Au(0));
+            self.base.block_container_explicit_block_size;
         for fragment in self.fragments.fragments.iter_mut() {
             fragment.update_late_computed_replaced_inline_size_if_necessary();
             fragment.assign_replaced_block_size_if_necessary(

components/layout/list_item.rs

@@ -106,7 +106,7 @@ impl Flow for ListItemFlow {
 
         if let Some(ref mut marker) = self.marker {
             let containing_block_block_size =
-                self.block_flow.base.block_container_explicit_block_size.unwrap_or(Au(0));
+                self.block_flow.base.block_container_explicit_block_size;
             marker.assign_replaced_block_size_if_necessary(containing_block_block_size);
 
             let font_metrics =

components/layout/model.rs

@@ -126,13 +126,20 @@ impl MarginCollapseInfo {
 
     pub fn finish_and_compute_collapsible_margins(mut self,
                                                   fragment: &Fragment,
+                                                  containing_block_size: Option<Au>,
                                                   can_collapse_block_end_margin_with_kids: bool)
                                                   -> (CollapsibleMargins, Au) {
         let state = match self.state {
             MarginCollapseState::AccumulatingCollapsibleTopMargin => {
-                match fragment.style().content_block_size() {
+                let may_collapse_through = match fragment.style().content_block_size() {
-                    LengthOrPercentageOrAuto::Auto | LengthOrPercentageOrAuto::Length(Au(0)) |
+                    LengthOrPercentageOrAuto::Auto => true,
-                    LengthOrPercentageOrAuto::Percentage(0.) => {
+                    LengthOrPercentageOrAuto::Length(Au(0)) => true,
+                    LengthOrPercentageOrAuto::Percentage(0.) => true,
+                    LengthOrPercentageOrAuto::Percentage(_) if containing_block_size.is_none() => true,
+                    _ => false,
+                };
+
+                if may_collapse_through {
                     match fragment.style().min_block_size() {
                         LengthOrPercentage::Length(Au(0)) | LengthOrPercentage::Percentage(0.) => {
                             FinalMarginState::MarginsCollapseThrough
@@ -143,14 +150,12 @@ impl MarginCollapseInfo {
                             FinalMarginState::BottomMarginCollapses
                         }
                     }
-                    },
+                } else {
-                    _ => {
                     // If the fragment has an explicitly specified block-size, margins may not
                     // collapse through it.
                     FinalMarginState::BottomMarginCollapses
                 }
             }
-            }
             MarginCollapseState::AccumulatingMarginIn => FinalMarginState::BottomMarginCollapses,
         };
 

components/script/dom/element.rs

@@ -390,15 +390,15 @@ impl RawLayoutElementHelpers for Element {
         match height {
             LengthOrPercentageOrAuto::Auto => {}
             LengthOrPercentageOrAuto::Percentage(percentage) => {
-                let width_value = specified::LengthOrPercentageOrAuto::Percentage(percentage);
+                let height_value = specified::LengthOrPercentageOrAuto::Percentage(percentage);
-                hints.push(from_declaration(PropertyDeclaration::Height(SpecifiedValue(
+                hints.push(from_declaration(
-                                height::SpecifiedValue(width_value)))));
+                    PropertyDeclaration::Height(SpecifiedValue(height_value))));
             }
             LengthOrPercentageOrAuto::Length(length) => {
-                let width_value = specified::LengthOrPercentageOrAuto::Length(
+                let height_value = specified::LengthOrPercentageOrAuto::Length(
                     specified::Length::Absolute(length));
-                hints.push(from_declaration(PropertyDeclaration::Height(SpecifiedValue(
+                hints.push(from_declaration(
-                                height::SpecifiedValue(width_value)))));
+                    PropertyDeclaration::Height(SpecifiedValue(height_value))));
             }
         }
 
@@ -443,8 +443,7 @@ impl RawLayoutElementHelpers for Element {
             let value = specified::Length::FontRelative(specified::FontRelativeLength::Em(rows as CSSFloat));
             hints.push(from_declaration(
                 PropertyDeclaration::Height(SpecifiedValue(
-                    longhands::height::SpecifiedValue(
+                        specified::LengthOrPercentageOrAuto::Length(value)))));
-                        specified::LengthOrPercentageOrAuto::Length(value))))));
         }
 
 

components/style/properties.mako.rs

@@ -534,45 +534,10 @@ pub mod longhands {
     ${predefined_type("width", "LengthOrPercentageOrAuto",
                       "computed::LengthOrPercentageOrAuto::Auto",
                       "parse_non_negative")}
-    <%self:longhand name="height">
-        use values::computed::Context;
-        use cssparser::ToCss;
-        use std::fmt;
 
-        impl ToCss for SpecifiedValue {
+    ${predefined_type("height", "LengthOrPercentageOrAuto",
-            fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+                      "computed::LengthOrPercentageOrAuto::Auto",
-                self.0.to_css(dest)
+                      "parse_non_negative")}
-            }
-        }
-
-        #[derive(Clone, PartialEq)]
-        pub struct SpecifiedValue(pub specified::LengthOrPercentageOrAuto);
-        pub mod computed_value {
-            pub use values::computed::LengthOrPercentageOrAuto as T;
-        }
-        #[inline]
-        pub fn get_initial_value() -> computed_value::T { computed::LengthOrPercentageOrAuto::Auto }
-        #[inline]
-        pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
-            specified::LengthOrPercentageOrAuto::parse_non_negative(input).map(SpecifiedValue)
-        }
-
-        impl ToComputedValue for SpecifiedValue {
-            type ComputedValue = computed_value::T;
-
-            #[inline]
-            fn to_computed_value(&self, context: &Context) -> computed_value::T {
-                match (self.0, context.inherited_height) {
-                    (specified::LengthOrPercentageOrAuto::Percentage(_),
-                     computed::LengthOrPercentageOrAuto::Auto)
-                    if !context.is_root_element && !context.positioned => {
-                        computed::LengthOrPercentageOrAuto::Auto
-                    },
-                    _ => self.0.to_computed_value(context)
-                }
-            }
-        }
-    </%self:longhand>
 
     ${predefined_type("min-width", "LengthOrPercentage",
                       "computed::LengthOrPercentage::Length(Au(0))",
@@ -6217,7 +6182,6 @@ pub fn cascade(viewport_size: Size2D<Au>,
             viewport_size: viewport_size,
             inherited_font_weight: inherited_font_style.font_weight,
             inherited_font_size: inherited_font_style.font_size,
-            inherited_height: inherited_style.get_box().height,
             inherited_text_decorations_in_effect:
                 inherited_style.get_inheritedtext()._servo_text_decorations_in_effect,
             // To be overridden by applicable declarations:

components/style/values.rs

@@ -873,7 +873,6 @@ pub mod computed {
         pub inherited_font_size: longhands::font_size::computed_value::T,
         pub inherited_text_decorations_in_effect:
             longhands::_servo_text_decorations_in_effect::computed_value::T,
-        pub inherited_height: longhands::height::computed_value::T,
         pub color: longhands::color::computed_value::T,
         pub text_decoration: longhands::text_decoration::computed_value::T,
         pub font_size: longhands::font_size::computed_value::T,

tests/wpt/metadata-css/css21_dev/html4/block-in-inline-percents-001.htm.ini

@@ -1,3 +0,0 @@
-[block-in-inline-percents-001.htm]
-  type: reftest
-  expected: FAIL