Auto merge of #5623 - pcwalton:nested-inline-vertical-align, r=glennw

This allows things like `<sup><span>Foo</span></sup>` to work and
improves Wikipedia.

r? @glennw

components/layout/construct.rs

@@ -651,7 +651,7 @@ impl<'a> FlowConstructor<'a> {
     /// `InlineFragmentsConstructionResult` if this node consisted entirely of ignorable
     /// whitespace.
     fn build_fragments_for_nonreplaced_inline_content(&mut self, node: &ThreadSafeLayoutNode)
-                                                  -> ConstructionResult {
+                                                      -> ConstructionResult {
         let mut opt_inline_block_splits: LinkedList<InlineBlockSplit> = LinkedList::new();
         let mut fragment_accumulator = InlineFragmentsAccumulator::from_inline_node(node);
         let mut abs_descendants = Descendants::new();
@@ -760,13 +760,10 @@ impl<'a> FlowConstructor<'a> {
                 node.restyle_damage()))
         }
 
-        // If the value of `display` property is not `inline`, then we have a situation like
-        // `<div style="position:absolute">foo bar baz</div>`. The fragments for `foo`, `bar`, and
-        // `baz` had better not be absolutely positioned!
+        // Modify the style as necessary. (See the comment in
+        // `properties::modify_style_for_replaced_content()`.)
         let mut style = (*node.style()).clone();
-        if style.get_box().display != display::T::inline {
-            style = Arc::new(properties::make_inline(&*style))
-        }
+        properties::modify_style_for_replaced_content(&mut style);
 
         // If this is generated content, then we need to initialize the accumulator with the
         // fragment corresponding to that content. Otherwise, just initialize with the ordinary

components/layout/fragment.rs

@@ -42,9 +42,8 @@ use std::sync::mpsc::Sender;
 use std::sync::{Arc, Mutex};
 use string_cache::Atom;
 use style::computed_values::content::ContentItem;
-use style::computed_values::{clear, mix_blend_mode, overflow_wrap};
-use style::computed_values::{position, text_align, text_decoration, vertical_align, white_space};
-use style::computed_values::{word_break};
+use style::computed_values::{clear, mix_blend_mode, overflow_wrap, position, text_align};
+use style::computed_values::{text_decoration, white_space, word_break};
 use style::node::{TElement, TNode};
 use style::properties::{ComputedValues, cascade_anonymous, make_border};
 use style::values::computed::{LengthOrPercentage, LengthOrPercentageOrAuto};
@@ -108,8 +107,8 @@ pub struct Fragment {
     /// Info specific to the kind of fragment. Keep this enum small.
     pub specific: SpecificFragmentInfo,
 
-    /// Holds the style context information for fragments
-    /// that are part of an inline formatting context.
+    /// Holds the style context information for fragments that are part of an inline formatting
+    /// context.
     pub inline_context: Option<InlineFragmentContext>,
 
     /// How damaged this fragment is since last reflow.
@@ -815,8 +814,8 @@ impl Fragment {
         self.restyle_damage | self.specific.restyle_damage()
     }
 
-    /// Adds a style to the inline context for this fragment. If the inline
-    /// context doesn't exist yet, it will be created.
+    /// Adds a style to the inline context for this fragment. If the inline context doesn't exist
+    /// yet, it will be created.
     pub fn add_inline_context_style(&mut self,
                                     style: Arc<ComputedValues>,
                                     first_frag: bool,
@@ -1070,16 +1069,14 @@ impl Fragment {
             LogicalSize::zero(self.style.writing_mode)
         };
 
-        match self.inline_context {
-            None => {}
-            Some(ref inline_fragment_context) => {
-                for style in inline_fragment_context.styles.iter() {
-                    if style.get_box().position == position::T::relative {
-                        rel_pos = rel_pos + from_style(&**style, containing_block_size);
-                    }
+        if let Some(ref inline_fragment_context) = self.inline_context {
+            for style in inline_fragment_context.styles.iter() {
+                if style.get_box().position == position::T::relative {
+                    rel_pos = rel_pos + from_style(&**style, containing_block_size);
                 }
-            },
+            }
         }
+
         rel_pos
     }
 
@@ -1108,10 +1105,6 @@ impl Fragment {
         self.style().get_inheritedtext().text_align
     }
 
-    pub fn vertical_align(&self) -> vertical_align::T {
-        self.style().get_box().vertical_align
-    }
-
     pub fn white_space(&self) -> white_space::T {
         self.style().get_inheritedtext().white_space
     }
@@ -1218,16 +1211,13 @@ impl Fragment {
 
         // Take borders and padding for parent inline fragments into account, if necessary.
         if self.is_primary_fragment() {
-            match self.inline_context {
-                None => {}
-                Some(ref context) => {
-                    for style in context.styles.iter() {
-                        let border_width = style.logical_border_width().inline_start_end();
-                        let padding_inline_size =
-                            model::padding_from_style(&**style, Au(0)).inline_start_end();
-                        result.surrounding_size = result.surrounding_size + border_width +
-                            padding_inline_size;
-                    }
+            if let Some(ref context) = self.inline_context {
+                for style in context.styles.iter() {
+                    let border_width = style.logical_border_width().inline_start_end();
+                    let padding_inline_size =
+                        model::padding_from_style(&**style, Au(0)).inline_start_end();
+                    result.surrounding_size = result.surrounding_size + border_width +
+                        padding_inline_size;
                 }
             }
         }
@@ -1955,6 +1945,10 @@ impl Fragment {
         scanned_text_fragment_info.range.adjust_by(CharIndex(leading_whitespace_character_count),
                                                    -CharIndex(leading_whitespace_character_count));
     }
+
+    pub fn inline_styles<'a>(&'a self) -> InlineStyleIterator<'a> {
+        InlineStyleIterator::new(self)
+    }
 }
 
 impl fmt::Debug for Fragment {
@@ -2009,3 +2003,40 @@ pub enum CoordinateSystem {
     Own,
 }
 
+pub struct InlineStyleIterator<'a> {
+    fragment: &'a Fragment,
+    inline_style_index: usize,
+    primary_style_yielded: bool,
+}
+
+impl<'a> Iterator for InlineStyleIterator<'a> {
+    type Item = &'a ComputedValues;
+
+    fn next(&mut self) -> Option<&'a ComputedValues> {
+        if !self.primary_style_yielded {
+            self.primary_style_yielded = true;
+            return Some(&*self.fragment.style)
+        }
+        let inline_context = match self.fragment.inline_context {
+            None => return None,
+            Some(ref inline_context) => inline_context,
+        };
+        let inline_style_index = self.inline_style_index;
+        if inline_style_index == inline_context.styles.len() {
+            return None
+        }
+        self.inline_style_index += 1;
+        Some(&*inline_context.styles[inline_style_index])
+    }
+}
+
+impl<'a> InlineStyleIterator<'a> {
+    fn new<'b>(fragment: &'b Fragment) -> InlineStyleIterator<'b> {
+        InlineStyleIterator {
+            fragment: fragment,
+            inline_style_index: 0,
+            primary_style_yielded: false,
+        }
+    }
+}
+

components/layout/inline.rs

@@ -24,13 +24,14 @@ use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::TextRun;
 use std::cmp::max;
 use std::fmt;
+use std::iter;
 use std::mem;
 use std::num::ToPrimitive;
 use std::ops::{Add, Sub, Mul, Div, Rem, Neg, Shl, Shr, Not, BitOr, BitAnd, BitXor};
 use std::sync::Arc;
 use std::u16;
-use style::computed_values::{overflow_x, text_align, text_justify, text_overflow, vertical_align};
-use style::computed_values::{white_space};
+use style::computed_values::{display, overflow_x, text_align, text_justify, text_overflow};
+use style::computed_values::{vertical_align, white_space};
 use style::properties::ComputedValues;
 use util::geometry::{Au, MAX_AU, ZERO_RECT};
 use util::logical_geometry::{LogicalRect, LogicalSize, WritingMode};
@@ -809,59 +810,80 @@ impl InlineFlow {
                               largest_block_size_for_bottom_fragments: &mut Au,
                               layout_context: &LayoutContext)
                               -> (Au, bool) {
-        match fragment.vertical_align() {
-            vertical_align::T::baseline => (-ascent, false),
-            vertical_align::T::middle => {
-                // TODO: x-height value should be used from font info.
-                // TODO: The code below passes our current reftests but doesn't work in all
-                // situations. Add vertical align reftests and fix this.
-                (-ascent, false)
-            },
-            vertical_align::T::sub => {
-                let sub_offset = (parent_text_block_start + parent_text_block_end)
-                                    .scale_by(FONT_SUBSCRIPT_OFFSET_RATIO);
-                (sub_offset - ascent, false)
-            },
-            vertical_align::T::super_ => {
-                let super_offset = (parent_text_block_start + parent_text_block_end)
-                                    .scale_by(FONT_SUPERSCRIPT_OFFSET_RATIO);
-                (-super_offset - ascent, false)
-            },
-            vertical_align::T::text_top => {
-                let fragment_block_size = *block_size_above_baseline + *depth_below_baseline;
-                let prev_depth_below_baseline = *depth_below_baseline;
-                *block_size_above_baseline = parent_text_block_start;
-                *depth_below_baseline = fragment_block_size - *block_size_above_baseline;
-                (*depth_below_baseline - prev_depth_below_baseline - ascent, false)
-            },
-            vertical_align::T::text_bottom => {
-                let fragment_block_size = *block_size_above_baseline + *depth_below_baseline;
-                let prev_depth_below_baseline = *depth_below_baseline;
-                *depth_below_baseline = parent_text_block_end;
-                *block_size_above_baseline = fragment_block_size - *depth_below_baseline;
-                (*depth_below_baseline - prev_depth_below_baseline - ascent, false)
-            },
-            vertical_align::T::top => {
-                *largest_block_size_for_top_fragments =
-                    max(*largest_block_size_for_top_fragments,
-                        *block_size_above_baseline + *depth_below_baseline);
-                let offset_top = *block_size_above_baseline - ascent;
-                (offset_top, true)
-            },
-            vertical_align::T::bottom => {
-                *largest_block_size_for_bottom_fragments =
-                    max(*largest_block_size_for_bottom_fragments,
-                        *block_size_above_baseline + *depth_below_baseline);
-                let offset_bottom = -(*depth_below_baseline + ascent);
-                (offset_bottom, true)
-            },
-            vertical_align::T::Length(length) => (-(length + ascent), false),
-            vertical_align::T::Percentage(p) => {
-                let line_height = fragment.calculate_line_height(layout_context);
-                let percent_offset = line_height.scale_by(p);
-                (-(percent_offset + ascent), false)
+        let (mut offset_from_baseline, mut largest_size_updated) = (Au(0), false);
+        for style in fragment.inline_styles() {
+            // Ignore `vertical-align` values for table cells.
+            let box_style = style.get_box();
+            if box_style.display != display::T::inline &&
+                    box_style.display != display::T::block {
+                continue
+            }
+
+            match box_style.vertical_align {
+                vertical_align::T::baseline => {}
+                vertical_align::T::middle => {
+                    // TODO: x-height value should be used from font info.
+                    // TODO: Doing nothing here passes our current reftests but doesn't work in
+                    // all situations. Add vertical align reftests and fix this.
+                },
+                vertical_align::T::sub => {
+                    let sub_offset = (parent_text_block_start + parent_text_block_end)
+                                        .scale_by(FONT_SUBSCRIPT_OFFSET_RATIO);
+                    offset_from_baseline = offset_from_baseline + sub_offset
+                },
+                vertical_align::T::super_ => {
+                    let super_offset = (parent_text_block_start + parent_text_block_end)
+                                        .scale_by(FONT_SUPERSCRIPT_OFFSET_RATIO);
+                    offset_from_baseline = offset_from_baseline - super_offset
+                },
+                vertical_align::T::text_top => {
+                    let fragment_block_size = *block_size_above_baseline +
+                        *depth_below_baseline;
+                    let prev_depth_below_baseline = *depth_below_baseline;
+                    *block_size_above_baseline = parent_text_block_start;
+                    *depth_below_baseline = fragment_block_size - *block_size_above_baseline;
+                    offset_from_baseline = offset_from_baseline + *depth_below_baseline -
+                        prev_depth_below_baseline
+                },
+                vertical_align::T::text_bottom => {
+                    let fragment_block_size = *block_size_above_baseline +
+                        *depth_below_baseline;
+                    let prev_depth_below_baseline = *depth_below_baseline;
+                    *depth_below_baseline = parent_text_block_end;
+                    *block_size_above_baseline = fragment_block_size - *depth_below_baseline;
+                    offset_from_baseline = offset_from_baseline + *depth_below_baseline -
+                        prev_depth_below_baseline
+                },
+                vertical_align::T::top => {
+                    if !largest_size_updated {
+                        largest_size_updated = true;
+                        *largest_block_size_for_top_fragments =
+                            max(*largest_block_size_for_top_fragments,
+                                *block_size_above_baseline + *depth_below_baseline);
+                        offset_from_baseline = offset_from_baseline +
+                            *block_size_above_baseline
+                    }
+                },
+                vertical_align::T::bottom => {
+                    if !largest_size_updated {
+                        largest_size_updated = true;
+                        *largest_block_size_for_bottom_fragments =
+                            max(*largest_block_size_for_bottom_fragments,
+                                *block_size_above_baseline + *depth_below_baseline);
+                        offset_from_baseline = offset_from_baseline - *depth_below_baseline
+                    }
+                },
+                vertical_align::T::Length(length) => {
+                    offset_from_baseline = offset_from_baseline - length
+                }
+                vertical_align::T::Percentage(p) => {
+                    let line_height = fragment.calculate_line_height(layout_context);
+                    let percent_offset = line_height.scale_by(p);
+                    offset_from_baseline = offset_from_baseline - percent_offset
+                }
             }
         }
+        (offset_from_baseline - ascent, largest_size_updated)
     }
 
     /// Sets fragment positions in the inline direction based on alignment for one line. This
@@ -989,22 +1011,46 @@ impl InlineFlow {
                                     baseline_distance_from_block_start: Au,
                                     largest_depth_below_baseline: Au) {
         for fragment_index in range(line.range.begin(), line.range.end()) {
+            // If any of the inline styles say `top` or `bottom`, adjust the vertical align
+            // appropriately.
+            //
+            // FIXME(#5624, pcwalton): This passes our current reftests but isn't the right thing
+            // to do.
             let fragment = fragments.get_mut(fragment_index.to_usize());
-            match fragment.vertical_align() {
+            let mut vertical_align = vertical_align::T::baseline;
+            for style in fragment.inline_styles() {
+                match (style.get_box().display, style.get_box().vertical_align) {
+                    (display::T::inline, vertical_align::T::top) |
+                    (display::T::block, vertical_align::T::top) => {
+                        vertical_align = vertical_align::T::top;
+                        break
+                    }
+                    (display::T::inline, vertical_align::T::bottom) |
+                    (display::T::block, vertical_align::T::bottom) => {
+                        vertical_align = vertical_align::T::bottom;
+                        break
+                    }
+                    _ => {}
+                }
+            }
+
+            match vertical_align {
                 vertical_align::T::top => {
                     fragment.border_box.start.b = fragment.border_box.start.b +
                         line_distance_from_flow_block_start
                 }
                 vertical_align::T::bottom => {
                     fragment.border_box.start.b = fragment.border_box.start.b +
-                        line_distance_from_flow_block_start + baseline_distance_from_block_start +
-                        largest_depth_below_baseline
+                        line_distance_from_flow_block_start +
+                        baseline_distance_from_block_start +
+                        largest_depth_below_baseline;
                 }
                 _ => {
                     fragment.border_box.start.b = fragment.border_box.start.b +
                         line_distance_from_flow_block_start + baseline_distance_from_block_start
                 }
             }
+
             fragment.update_late_computed_block_position_if_necessary();
         }
     }

components/style/properties.mako.rs

@@ -5239,13 +5239,27 @@ pub fn cascade_anonymous(parent_style: &ComputedValues) -> ComputedValues {
     result
 }
 
-/// Sets `display` to `inline` and `position` to `static`.
+/// Alters the given style to accommodate replaced content. This is called in flow construction. It
+/// handles cases like `<div style="position: absolute">foo bar baz</div>` (in which `foo`, `bar`,
+/// and `baz` must not be absolutely-positioned) and cases like `<sup>Foo</sup>` (in which the
+/// `vertical-align: top` style of `sup` must not propagate down into `Foo`).
+///
+/// FIXME(#5625, pcwalton): It would probably be cleaner and faster to do this in the cascade.
 #[inline]
-pub fn make_inline(style: &ComputedValues) -> ComputedValues {
-    let mut style = (*style).clone();
-    style.box_.make_unique().display = longhands::display::computed_value::T::inline;
-    style.box_.make_unique().position = longhands::position::computed_value::T::static_;
-    style
+pub fn modify_style_for_replaced_content(style: &mut Arc<ComputedValues>) {
+    // Reset `position` to handle cases like `<div style="position: absolute">foo bar baz</div>`.
+    if style.box_.display != longhands::display::computed_value::T::inline {
+        let mut style = style.make_unique();
+        style.box_.make_unique().display = longhands::display::computed_value::T::inline;
+        style.box_.make_unique().position = longhands::position::computed_value::T::static_;
+    }
+
+    // Reset `vertical-align` to handle cases like `<sup>foo</sup>`.
+    if style.box_.vertical_align != longhands::vertical_align::computed_value::T::baseline {
+        let mut style = style.make_unique();
+        style.box_.make_unique().vertical_align =
+            longhands::vertical_align::computed_value::T::baseline
+    }
 }
 
 /// Sets `border_${side}_width` to the passed in values.

tests/ref/basic.list

@@ -311,8 +311,10 @@ experimental == rtl_simple.html rtl_simple_ref.html
 == upper_id_attr.html upper_id_attr_ref.html
 flaky_cpu,experimental == vertical-lr-blocks.html vertical-lr-blocks_ref.html
 == vertical_align_bottom_a.html vertical_align_bottom_ref.html
+== vertical_align_inside_table_a.html vertical_align_inside_table_ref.html
 == vertical_align_sub_a.html vertical_align_sub_ref.html
 == vertical_align_super_a.html vertical_align_super_ref.html
+== vertical_align_super_nested_a.html vertical_align_super_nested_ref.html
 == vertical_align_text_bottom_a.html vertical_align_text_bottom_ref.html
 == vertical_align_text_top_a.html vertical_align_text_top_ref.html
 == vertical_align_top_a.html vertical_align_top_ref.html

tests/ref/vertical_align_inside_table_a.html

@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<head>
+<!--
+    Tests that layout doesn't treat `vertical-align` on table cells as though it were
+    `vertical-align` on inlines.
+-->
+<style>
+td {
+    vertical-align: top;
+}
+</style>
+</head>
+<body>
+<table>
+<tr><td>What? <a>What?</a></td></tr>
+</table>
+</body>
+</html>
+

tests/ref/vertical_align_inside_table_ref.html

@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<!--
+    Tests that layout doesn't treat `vertical-align` on table cells as though it were
+    `vertical-align` on inlines.
+-->
+</head>
+<body>
+<table>
+<tr><td>What? <a>What?</a></td></tr>
+</table>
+</body>
+</html>
+

tests/ref/vertical_align_super_nested_a.html

@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+<head>
+<!-- Tests that `vertical-align: baseline` inside `vertical-align: super` is laid out properly. -->
+<style>
+span {
+    vertical-align: baseline;
+}
+</style>
+</head>
+<body>
+<p><sup><span>Yo</span></sup></p>
+</body>
+</html>
+

tests/ref/vertical_align_super_nested_ref.html

@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+<head>
+<!-- Tests that `vertical-align: baseline` inside `vertical-align: super` is laid out properly. -->
+</head>
+<body>
+<p><sup>Yo</sup></p>
+</body>
+</html>
+