Auto merge of #10643 - mbrubeck:empty-span-border, r=pcwalton

Fix handling of borders and padding for empty/stripped inline flows

This forces fragment generation for empty inline flows, if they have borders or padding.  Fixes #10533 and #2001.  Also includes fixes for other bugs that were uncovered by this change; see the individual commit messages for detailed explanations.  r? @pcwalton

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10643)
<!-- Reviewable:end -->

components/layout/construct.rs

@@ -13,6 +13,7 @@
 
 #![deny(unsafe_code)]
 
+use app_units::Au;
 use block::BlockFlow;
 use context::LayoutContext;
 use data::{HAS_NEWLY_CONSTRUCTED_FLOW, PrivateLayoutData};
@@ -808,7 +809,9 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
         let mut abs_descendants = AbsoluteDescendants::new();
 
         // Concatenate all the fragments of our kids, creating {ib} splits as necessary.
+        let mut is_empty = true;
         for kid in node.children() {
+            is_empty = false;
             if kid.get_pseudo_element_type() != PseudoElementType::Normal {
                 self.process(&kid);
             }
@@ -889,6 +892,22 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
             }
         }
 
+        if is_empty && node.style().has_padding_or_border() {
+            // An empty inline box needs at least one fragment to draw its background and borders.
+            let info = SpecificFragmentInfo::UnscannedText(
+                box UnscannedTextFragmentInfo::new(String::new(), None));
+            let mut modified_style = node.style().clone();
+            properties::modify_style_for_replaced_content(&mut modified_style);
+            properties::modify_style_for_text(&mut modified_style);
+            let fragment = Fragment::from_opaque_node_and_style(node.opaque(),
+                                                                node.get_pseudo_element_type().strip(),
+                                                                modified_style,
+                                                                node.selected_style().clone(),
+                                                                node.restyle_damage(),
+                                                                info);
+            fragment_accumulator.fragments.fragments.push_back(fragment)
+        }
+
         // Finally, make a new construction result.
         if opt_inline_block_splits.len() > 0 || !fragment_accumulator.fragments.is_empty()
                 || abs_descendants.len() > 0 {
@@ -923,8 +942,6 @@ impl<'a, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode>
         }
 
         // If this node is ignorable whitespace, bail out now.
-        //
-        // FIXME(#2001, pcwalton): Don't do this if there's padding or borders.
         if node.is_ignorable_whitespace() {
             return ConstructionResult::ConstructionItem(ConstructionItem::Whitespace(
                 node.opaque(),
@@ -1779,28 +1796,7 @@ pub fn strip_ignorable_whitespace_from_start(this: &mut LinkedList<Fragment>) {
             WhitespaceStrippingResult::FragmentContainedOnlyWhitespace => {
                 let removed_fragment = this.pop_front().unwrap();
                 if let Some(ref mut remaining_fragment) = this.front_mut() {
-                    if let Some(ref mut inline_context_of_remaining_fragment) =
+                    remaining_fragment.meld_with_prev_inline_fragment(&removed_fragment);
-                            remaining_fragment.inline_context {
-                        if let Some(ref inline_context_of_removed_fragment) =
-                                removed_fragment.inline_context {
-                            for (inline_context_node_from_removed_fragment,
-                                 inline_context_node_from_remaining_fragment) in
-                                    inline_context_of_removed_fragment.nodes.iter().rev().zip(
-                                        inline_context_of_remaining_fragment.nodes.iter_mut().rev()
-                                    ) {
-                                if !inline_context_node_from_removed_fragment.flags.contains(
-                                        FIRST_FRAGMENT_OF_ELEMENT) {
-                                    continue
-                                }
-                                if inline_context_node_from_removed_fragment.address !=
-                                        inline_context_node_from_remaining_fragment.address {
-                                    continue
-                                }
-                                inline_context_node_from_remaining_fragment.flags.insert(
-                                    FIRST_FRAGMENT_OF_ELEMENT);
-                            }
-                        }
-                    }
                 }
             }
         }
@@ -1865,6 +1861,7 @@ fn control_chars_to_fragment(node: &InlineFragmentNodeInfo,
     let info = SpecificFragmentInfo::UnscannedText(
         box UnscannedTextFragmentInfo::new(String::from(text), None));
     let mut style = node.style.clone();
+    properties::modify_style_for_replaced_content(&mut style);
     properties::modify_style_for_text(&mut style);
     Fragment::from_opaque_node_and_style(node.address,
                                          node.pseudo,
@@ -1873,3 +1870,25 @@ fn control_chars_to_fragment(node: &InlineFragmentNodeInfo,
                                          restyle_damage,
                                          info)
 }
+
+/// Convenience methods for computed CSS values
+trait ComputedValueUtils {
+    /// Returns true if this node has non-zero padding or border.
+    fn has_padding_or_border(&self) -> bool;
+}
+
+impl ComputedValueUtils for ServoComputedValues {
+    fn has_padding_or_border(&self) -> bool {
+        let padding = self.get_padding();
+        let border = self.get_border();
+
+        !padding.padding_top.is_definitely_zero() ||
+           !padding.padding_right.is_definitely_zero() ||
+           !padding.padding_bottom.is_definitely_zero() ||
+           !padding.padding_left.is_definitely_zero() ||
+           border.border_top_width != Au(0) ||
+           border.border_right_width != Au(0) ||
+           border.border_bottom_width != Au(0) ||
+           border.border_left_width != Au(0)
+    }
+}

components/layout/fragment.rs

@@ -228,11 +228,11 @@ impl fmt::Debug for SpecificFragmentInfo {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
             SpecificFragmentInfo::ScannedText(ref info) => {
-                write!(f, "\"{}\"", slice_chars(&*info.run.text, info.range.begin().get() as usize,
+                write!(f, "{:?}", slice_chars(&*info.run.text, info.range.begin().get() as usize,
                                                  info.range.end().get() as usize))
             }
             SpecificFragmentInfo::UnscannedText(ref info) => {
-                write!(f, "\"{}\"", info.text)
+                write!(f, "{:?}", info.text)
             }
             _ => Ok(())
         }
@@ -2497,26 +2497,49 @@ impl Fragment {
     pub fn meld_with_next_inline_fragment(&mut self, next_fragment: &Fragment) {
         if let Some(ref mut inline_context_of_this_fragment) = self.inline_context {
             if let Some(ref inline_context_of_next_fragment) = next_fragment.inline_context {
-                for (i, inline_context_node_from_next_fragment) in
+                for (inline_context_node_from_this_fragment,
-                        inline_context_of_next_fragment.nodes.iter().enumerate() {
+                     inline_context_node_from_next_fragment)
-                    if i >= inline_context_of_this_fragment.nodes.len() {
+                    in inline_context_of_this_fragment.nodes.iter_mut().rev()
-                        continue
+                        .zip(inline_context_of_next_fragment.nodes.iter().rev())
-                    }
+                {
                     if !inline_context_node_from_next_fragment.flags.contains(
                             LAST_FRAGMENT_OF_ELEMENT) {
                         continue
                     }
                     if inline_context_node_from_next_fragment.address !=
-                            inline_context_of_this_fragment.nodes[i].address {
+                            inline_context_node_from_this_fragment.address {
                         continue
                     }
-                    inline_context_of_this_fragment.nodes[i].flags.insert(
+                    inline_context_node_from_this_fragment.flags.insert(LAST_FRAGMENT_OF_ELEMENT);
-                        LAST_FRAGMENT_OF_ELEMENT);
                 }
             }
         }
     }
 
+    pub fn meld_with_prev_inline_fragment(&mut self, prev_fragment: &Fragment) {
+        if let Some(ref mut inline_context_of_this_fragment) = self.inline_context {
+            if let Some(ref inline_context_of_prev_fragment) = prev_fragment.inline_context {
+                for (inline_context_node_from_prev_fragment,
+                     inline_context_node_from_this_fragment)
+                    in inline_context_of_prev_fragment.nodes.iter().rev().zip(
+                            inline_context_of_this_fragment.nodes.iter_mut().rev())
+                {
+                    if !inline_context_node_from_prev_fragment.flags.contains(
+                            FIRST_FRAGMENT_OF_ELEMENT) {
+                        continue
+                    }
+                    if inline_context_node_from_prev_fragment.address !=
+                            inline_context_node_from_this_fragment.address {
+                        continue
+                    }
+                    inline_context_node_from_this_fragment.flags.insert(
+                        FIRST_FRAGMENT_OF_ELEMENT);
+                }
+            }
+        }
+    }
+
+
     pub fn fragment_id(&self) -> usize {
         return self as *const Fragment as usize;
     }

components/layout/incremental.rs

@@ -5,7 +5,7 @@
 use flow::{self, AFFECTS_COUNTERS, Flow, HAS_COUNTER_AFFECTING_CHILDREN, IS_ABSOLUTELY_POSITIONED};
 use std::fmt;
 use std::sync::Arc;
-use style::computed_values::float;
+use style::computed_values::{display, float};
 use style::dom::TRestyleDamage;
 use style::properties::{ComputedValues, ServoComputedValues};
 
@@ -193,7 +193,17 @@ pub fn compute_damage(old: Option<&Arc<ServoComputedValues>>, new: &ServoCompute
         get_text.text_decoration, get_text.unicode_bidi,
         get_inheritedtable.empty_cells, get_inheritedtable.caption_side,
         get_column.column_width, get_column.column_count
-    ]) || add_if_not_equal!(old, new, damage,
+    ]) || (new.get_box().display == display::T::inline &&
+           add_if_not_equal!(old, new, damage,
+                             [REPAINT, STORE_OVERFLOW, BUBBLE_ISIZES, REFLOW_OUT_OF_FLOW, REFLOW,
+                              RECONSTRUCT_FLOW], [
+        // For inline boxes only, border/padding styles are used in flow construction (to decide
+        // whether to create fragments for empty flows).
+        get_border.border_top_width, get_border.border_right_width,
+        get_border.border_bottom_width, get_border.border_left_width,
+        get_padding.padding_top, get_padding.padding_right,
+        get_padding.padding_bottom, get_padding.padding_left
+    ])) || add_if_not_equal!(old, new, damage,
                             [ REPAINT, STORE_OVERFLOW, BUBBLE_ISIZES, REFLOW_OUT_OF_FLOW, REFLOW ],
         [get_border.border_top_width, get_border.border_right_width,
         get_border.border_bottom_width, get_border.border_left_width,

components/layout/text.rs

@@ -15,7 +15,7 @@ use gfx::font_context::FontContext;
 use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::TextRun;
 use gfx::text::util::{self, CompressionMode};
-use inline::InlineFragments;
+use inline::{FIRST_FRAGMENT_OF_ELEMENT, InlineFragments, LAST_FRAGMENT_OF_ELEMENT};
 use range::{Range, RangeIndex};
 use std::borrow::ToOwned;
 use std::collections::LinkedList;
@@ -173,6 +173,7 @@ impl TextRunScanner {
             let mut insertion_point = None;
 
             for (fragment_index, in_fragment) in self.clump.iter().enumerate() {
+                debug!("  flushing {:?}", in_fragment);
                 let mut mapping = RunMapping::new(&run_info_list[..], &run_info, fragment_index);
                 let text;
                 let selection;
@@ -239,7 +240,6 @@ impl TextRunScanner {
                         mapping.flush(&mut mappings,
                                          &mut run_info,
                                          &**text,
-                                         insertion_point,
                                          compression,
                                          text_transform,
                                          &mut last_whitespace,
@@ -269,7 +269,6 @@ impl TextRunScanner {
                 mapping.flush(&mut mappings,
                               &mut run_info,
                               &**text,
-                              insertion_point,
                               compression,
                               text_transform,
                               &mut last_whitespace,
@@ -321,19 +320,26 @@ impl TextRunScanner {
         // Make new fragments with the runs and adjusted text indices.
         debug!("TextRunScanner: pushing {} fragment(s)", self.clump.len());
         let mut mappings = mappings.into_iter().peekable();
+        let mut prev_fragments_to_meld = Vec::new();
+
         for (logical_offset, old_fragment) in
                 mem::replace(&mut self.clump, LinkedList::new()).into_iter().enumerate() {
-             loop {
+            let mut is_first_mapping_of_this_old_fragment = true;
+            loop {
                 match mappings.peek() {
                     Some(mapping) if mapping.old_fragment_index == logical_offset => {}
                     Some(_) | None => {
-                        if let Some(ref mut last_fragment) = out_fragments.last_mut() {
+                        if is_first_mapping_of_this_old_fragment {
-                            last_fragment.meld_with_next_inline_fragment(&old_fragment);
+                            // There were no mappings for this unscanned fragment. Transfer its
+                            // flags to the previous/next sibling elements instead.
+                            if let Some(ref mut last_fragment) = out_fragments.last_mut() {
+                                last_fragment.meld_with_next_inline_fragment(&old_fragment);
+                            }
+                            prev_fragments_to_meld.push(old_fragment);
                         }
                         break;
                     }
                 };
-
                 let mut mapping = mappings.next().unwrap();
                 let scanned_run = runs[mapping.text_run_index].clone();
 
@@ -372,10 +378,31 @@ impl TextRunScanner {
                 let bounding_box_size = bounding_box_for_run_metrics(&new_metrics, writing_mode);
                 new_text_fragment_info.content_size = bounding_box_size;
 
-                let new_fragment = old_fragment.transform(
+                let mut new_fragment = old_fragment.transform(
                     bounding_box_size,
                     SpecificFragmentInfo::ScannedText(new_text_fragment_info));
 
+                let is_last_mapping_of_this_old_fragment = match mappings.peek() {
+                    Some(mapping) if mapping.old_fragment_index == logical_offset => false,
+                    _ => true
+                };
+
+                if let Some(ref mut context) = new_fragment.inline_context {
+                    for node in &mut context.nodes {
+                        if !is_last_mapping_of_this_old_fragment {
+                            node.flags.remove(LAST_FRAGMENT_OF_ELEMENT);
+                        }
+                        if !is_first_mapping_of_this_old_fragment {
+                            node.flags.remove(FIRST_FRAGMENT_OF_ELEMENT);
+                        }
+                    }
+                }
+
+                for prev_fragment in prev_fragments_to_meld.drain(..) {
+                    new_fragment.meld_with_prev_inline_fragment(&prev_fragment);
+                }
+
+                is_first_mapping_of_this_old_fragment = false;
                 out_fragments.push(new_fragment)
             }
         }
@@ -578,15 +605,12 @@ impl RunMapping {
              mappings: &mut Vec<RunMapping>,
              run_info: &mut RunInfo,
              text: &str,
-             insertion_point: Option<CharIndex>,
              compression: CompressionMode,
              text_transform: text_transform::T,
              last_whitespace: &mut bool,
              start_position: &mut usize,
              end_position: usize) {
-        if *start_position == end_position && insertion_point.is_none() {
+        let was_empty = *start_position == end_position;
-            return;
-        }
         let old_byte_length = run_info.text.len();
         *last_whitespace = util::transform_text(&text[(*start_position)..end_position],
                                                 compression,
@@ -605,8 +629,11 @@ impl RunMapping {
         run_info.character_length = run_info.character_length + character_count;
         *start_position = end_position;
 
-        // Don't flush mappings that contain no characters and no insertion_point.
+        // Don't save mappings that contain only discarded characters.
-        if character_count == 0 && !self.contains_insertion_point(insertion_point) {
+        // (But keep ones that contained no characters to begin with, since they might have been
+        // generated by an empty flow to draw its borders/padding/insertion point.)
+        let is_empty = character_count == 0;
+        if is_empty && !was_empty {
             return;
         }
 

components/style/values.rs

@@ -1662,9 +1662,22 @@ pub mod computed {
     }
 
     impl LengthOrPercentage {
+        #[inline]
         pub fn zero() -> LengthOrPercentage {
             LengthOrPercentage::Length(Au(0))
         }
+
+        /// Returns true if the computed value is absolute 0 or 0%.
+        ///
+        /// (Returns false for calc() values, even if ones that may resolve to zero.)
+        #[inline]
+        pub fn is_definitely_zero(&self) -> bool {
+            use self::LengthOrPercentage::*;
+            match *self {
+                Length(Au(0)) | Percentage(0.0) => true,
+                Length(_) | Percentage(_) | Calc(_) => false
+            }
+        }
     }
 
     impl fmt::Debug for LengthOrPercentage {

tests/wpt/metadata-css/css21_dev/html4/bidi-011.htm.ini

@@ -1,3 +0,0 @@
-[bidi-011.htm]
-  type: reftest
-  expected: FAIL