Auto merge of #7024 - pcwalton:whitespace-stripping, r=mbrubeck

layout: Rewrite whitespace stripping.

This patch makes Servo unconditionally strip whitespace before text run
scanning (assuming that the `white-space` property allows it). Whitespace
stripping during reflow is now only used for handling whitespace at the ends of
lines; reflow now never attempts to handle ignorable whitespace.

Many CSS tests pass now. There are some new failures, however.

The following reference tests now fail due to a pre-existing bug whereby
whitespace is used to calculate the position of inline hypothetical boxes for
elements with `display: inline; position: absolute`:

* `absolute-replaced-height-036.htm`
* `vertical-align-sub-001.htm`
* `vertical-align-super-001.htm`

The following reference tests fail due to a pre-existing bug whereby we don't
handle `font-size: 0` properly in inline reflow:

* `font-size-zero-1.htm`
* `font-size-zero-2.htm`

The following reference test fails due to the fact that it relied on our
incorrect insertion of whitespace to make room for the black background:

* `inline-formatting-context-007.htm`

r? @mbrubeck

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

components/gfx/text/util.rs

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#[derive(PartialEq, Eq, Copy, Clone)]
+#[derive(PartialEq, Eq, Copy, Clone, Debug)]
 pub enum CompressionMode {
     CompressNone,
     CompressWhitespace,

components/layout/construct.rs

@@ -26,6 +26,7 @@ use fragment::{Fragment, GeneratedContentInfo, IframeFragmentInfo};
 use fragment::{CanvasFragmentInfo, ImageFragmentInfo, InlineAbsoluteFragmentInfo};
 use fragment::{InlineAbsoluteHypotheticalFragmentInfo, TableColumnFragmentInfo};
 use fragment::{InlineBlockFragmentInfo, SpecificFragmentInfo, UnscannedTextFragmentInfo};
+use fragment::{WhitespaceStrippingResult};
 use incremental::{RECONSTRUCT_FLOW, RestyleDamage};
 use inline::{InlineFlow, InlineFragmentNodeInfo};
 use list_item::{ListItemFlow, ListStyleTypeContent};
@@ -58,6 +59,7 @@ use style::computed_values::{caption_side, display, empty_cells, float, list_sty
 use style::computed_values::{position};
 use style::properties::{self, ComputedValues};
 use url::Url;
+use util::linked_list;
 use util::opts;
 
 /// The results of flow construction for a DOM node.
@@ -266,12 +268,6 @@ impl InlineFragmentsAccumulator {
     }
 }
 
-enum WhitespaceStrippingMode {
-    None,
-    FromStart,
-    FromEnd,
-}
-
 /// An object that knows how to create flows.
 pub struct FlowConstructor<'a> {
     /// The layout context.
@@ -414,27 +410,16 @@ impl<'a> FlowConstructor<'a> {
                                               fragment_accumulator: InlineFragmentsAccumulator,
                                               flow: &mut FlowRef,
                                               flow_list: &mut Vec<FlowRef>,
-                                              whitespace_stripping: WhitespaceStrippingMode,
                                               node: &ThreadSafeLayoutNode) {
         let mut fragments = fragment_accumulator.to_intermediate_inline_fragments();
         if fragments.is_empty() {
             return
         };
 
-        match whitespace_stripping {
-            WhitespaceStrippingMode::None => {}
-            WhitespaceStrippingMode::FromStart => {
-                strip_ignorable_whitespace_from_start(&mut fragments.fragments);
-                if fragments.is_empty() {
-                    return
-                };
-            }
-            WhitespaceStrippingMode::FromEnd => {
-                strip_ignorable_whitespace_from_end(&mut fragments.fragments);
-                if fragments.is_empty() {
-                    return
-                };
-            }
+        strip_ignorable_whitespace_from_start(&mut fragments.fragments);
+        strip_ignorable_whitespace_from_end(&mut fragments.fragments);
+        if fragments.is_empty() {
+            return
         }
 
         // Build a list of all the inline-block fragments before fragments is moved.
@@ -504,8 +489,7 @@ impl<'a> FlowConstructor<'a> {
             node: &ThreadSafeLayoutNode,
             kid: ThreadSafeLayoutNode,
             inline_fragment_accumulator: &mut InlineFragmentsAccumulator,
-            abs_descendants: &mut Descendants,
-            first_fragment: &mut bool) {
+            abs_descendants: &mut Descendants) {
         match kid.swap_out_construction_result() {
             ConstructionResult::None => {}
             ConstructionResult::Flow(mut kid_flow, kid_abs_descendants) => {
@@ -527,7 +511,6 @@ impl<'a> FlowConstructor<'a> {
                                      InlineFragmentsAccumulator::new()),
                         flow,
                         consecutive_siblings,
-                        WhitespaceStrippingMode::FromStart,
                         node);
                     if !consecutive_siblings.is_empty() {
                         let consecutive_siblings = mem::replace(consecutive_siblings, vec!());
@@ -554,15 +537,6 @@ impl<'a> FlowConstructor<'a> {
                     } = split;
                     inline_fragment_accumulator.push_all(predecessors);
 
-                    // If this is the first fragment in flow, then strip ignorable
-                    // whitespace per CSS 2.1 § 9.2.1.1.
-                    let whitespace_stripping = if *first_fragment {
-                        *first_fragment = false;
-                        WhitespaceStrippingMode::FromStart
-                    } else {
-                        WhitespaceStrippingMode::None
-                    };
-
                     // Flush any inline fragments that we were gathering up.
                     debug!("flushing {} inline box(es) to flow A",
                            inline_fragment_accumulator.fragments.fragments.len());
@@ -571,7 +545,6 @@ impl<'a> FlowConstructor<'a> {
                                          InlineFragmentsAccumulator::new()),
                             flow,
                             consecutive_siblings,
-                            whitespace_stripping,
                             node);
 
                     // Push the flow generated by the {ib} split onto our list of
@@ -625,7 +598,6 @@ impl<'a> FlowConstructor<'a> {
         let mut consecutive_siblings = vec!();
 
         inline_fragment_accumulator.fragments.push_all(initial_fragments);
-        let mut first_fragment = inline_fragment_accumulator.fragments.is_empty();
 
         // List of absolute descendants, in tree order.
         let mut abs_descendants = Descendants::new();
@@ -640,16 +612,14 @@ impl<'a> FlowConstructor<'a> {
                 node,
                 kid,
                 &mut inline_fragment_accumulator,
-                &mut abs_descendants,
-                &mut first_fragment);
+                &mut abs_descendants);
         }
 
         // Perform a final flush of any inline fragments that we were gathering up to handle {ib}
         // splits, after stripping ignorable whitespace.
         self.flush_inline_fragments_to_flow_or_list(inline_fragment_accumulator,
                                                     &mut flow,
                                                     &mut consecutive_siblings,
-                                                    WhitespaceStrippingMode::FromEnd,
                                                     node);
         if !consecutive_siblings.is_empty() {
             self.generate_anonymous_missing_child(consecutive_siblings, &mut flow, node);
@@ -1665,10 +1635,21 @@ pub fn strip_ignorable_whitespace_from_start(this: &mut LinkedList<Fragment>) {
         return   // Fast path.
     }
 
-    while !this.is_empty() && this.front().as_ref().unwrap().is_ignorable_whitespace() {
-        debug!("stripping ignorable whitespace from start");
-        drop(this.pop_front());
+    let mut leading_fragments_consisting_of_solely_bidi_control_characters = LinkedList::new();
+    while !this.is_empty() {
+        match this.front_mut().as_mut().unwrap().strip_leading_whitespace_if_necessary() {
+            WhitespaceStrippingResult::RetainFragment => break,
+            WhitespaceStrippingResult::FragmentContainedOnlyBidiControlCharacters => {
+                leading_fragments_consisting_of_solely_bidi_control_characters.push_back(
+                    this.pop_front().unwrap())
+            }
+            WhitespaceStrippingResult::FragmentContainedOnlyWhitespace => {
+                this.pop_front();
+            }
+        }
     }
+    linked_list::prepend_from(this,
+                              &mut leading_fragments_consisting_of_solely_bidi_control_characters)
 }
 
 /// Strips ignorable whitespace from the end of a list of fragments.
@@ -1677,10 +1658,20 @@ pub fn strip_ignorable_whitespace_from_end(this: &mut LinkedList<Fragment>) {
         return
     }
 
-    while !this.is_empty() && this.back().as_ref().unwrap().is_ignorable_whitespace() {
-        debug!("stripping ignorable whitespace from end");
-        drop(this.pop_back());
+    let mut trailing_fragments_consisting_of_solely_bidi_control_characters = LinkedList::new();
+    while !this.is_empty() {
+        match this.back_mut().as_mut().unwrap().strip_trailing_whitespace_if_necessary() {
+            WhitespaceStrippingResult::RetainFragment => break,
+            WhitespaceStrippingResult::FragmentContainedOnlyBidiControlCharacters => {
+                trailing_fragments_consisting_of_solely_bidi_control_characters.push_front(
+                    this.pop_back().unwrap())
+            }
+            WhitespaceStrippingResult::FragmentContainedOnlyWhitespace => {
+                this.pop_back();
+            }
+        }
     }
+    this.append(&mut trailing_fragments_consisting_of_solely_bidi_control_characters);
 }
 
 /// If the 'unicode-bidi' property has a value other than 'normal', return the bidi control codes

components/layout/fragment.rs

@@ -23,6 +23,7 @@ use euclid::{Point2D, Rect, Size2D};
 use gfx::display_list::{BLUR_INFLATION_FACTOR, OpaqueNode};
 use gfx::text::glyph::CharIndex;
 use gfx::text::text_run::{TextRun, TextRunSlice};
+use gfx;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::{ConstellationChan, Msg, PipelineId, SubpageId};
 use net_traits::image::base::Image;
@@ -210,6 +211,9 @@ impl fmt::Debug for SpecificFragmentInfo {
                 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)
+            }
             _ => Ok(())
         }
     }
@@ -2113,35 +2117,144 @@ impl Fragment {
         }
     }
 
-    pub fn strip_leading_whitespace_if_necessary(&mut self) {
-        let mut scanned_text_fragment_info = match self.specific {
+    pub fn strip_leading_whitespace_if_necessary(&mut self) -> WhitespaceStrippingResult {
+        if self.style.get_inheritedtext().white_space == white_space::T::pre {
+            return WhitespaceStrippingResult::RetainFragment
+        }
+
+        match self.specific {
             SpecificFragmentInfo::ScannedText(ref mut scanned_text_fragment_info) => {
-                scanned_text_fragment_info
+                let mut leading_whitespace_character_count = 0;
+                {
+                    let text = slice_chars(
+                        &*scanned_text_fragment_info.run.text,
+                        scanned_text_fragment_info.range.begin().to_usize(),
+                        scanned_text_fragment_info.range.end().to_usize());
+                    for character in text.chars() {
+                        if util::str::char_is_whitespace(character) {
+                            leading_whitespace_character_count += 1
+                        } else {
+                            break
+                        }
+                    }
+                }
+
+                let whitespace_range = Range::new(scanned_text_fragment_info.range.begin(),
+                                                  CharIndex(leading_whitespace_character_count));
+                let text_bounds =
+                    scanned_text_fragment_info.run.metrics_for_range(&whitespace_range).bounding_box;
+                self.border_box.size.inline = self.border_box.size.inline - text_bounds.size.width;
+                scanned_text_fragment_info.content_size.inline =
+                    scanned_text_fragment_info.content_size.inline - text_bounds.size.width;
+
+                scanned_text_fragment_info.range.adjust_by(
+                    CharIndex(leading_whitespace_character_count),
+                    -CharIndex(leading_whitespace_character_count));
+
+                return WhitespaceStrippingResult::RetainFragment
+            }
+            SpecificFragmentInfo::UnscannedText(ref mut unscanned_text_fragment_info) => {
+                let mut new_text_string = String::new();
+                let mut modified = false;
+                for (i, character) in unscanned_text_fragment_info.text.char_indices() {
+                    if gfx::text::util::is_bidi_control(character) {
+                        new_text_string.push(character);
+                        continue
+                    }
+                    if util::str::char_is_whitespace(character) {
+                        modified = true;
+                        continue
+                    }
+                    new_text_string.push_str(&unscanned_text_fragment_info.text[i..]);
+                    break
+                }
+                if modified {
+                    unscanned_text_fragment_info.text = new_text_string.into_boxed_slice();
+                }
+
+                WhitespaceStrippingResult::from_unscanned_text_fragment_info(
+                    &unscanned_text_fragment_info)
             }
-            _ => return,
-        };
+            _ => WhitespaceStrippingResult::RetainFragment,
+        }
+    }
 
+    /// Returns true if the entire fragment was stripped.
+    pub fn strip_trailing_whitespace_if_necessary(&mut self) -> WhitespaceStrippingResult {
         if self.style.get_inheritedtext().white_space == white_space::T::pre {
-            return
-        }
-
-        let mut leading_whitespace_character_count = 0;
-        {
-            let text = slice_chars(
-                &*scanned_text_fragment_info.run.text,
-                scanned_text_fragment_info.range.begin().to_usize(),
-                scanned_text_fragment_info.range.end().to_usize());
-            for character in text.chars() {
-                if util::str::char_is_whitespace(character) {
-                    leading_whitespace_character_count += 1
-                } else {
+            return WhitespaceStrippingResult::RetainFragment
+        }
+
+        match self.specific {
+            SpecificFragmentInfo::ScannedText(ref mut scanned_text_fragment_info) => {
+                // FIXME(pcwalton): Is there a more clever (i.e. faster) way to do this?
+                debug!("stripping trailing whitespace: range={:?}, len={}",
+                       scanned_text_fragment_info.range,
+                       scanned_text_fragment_info.run.text.chars().count());
+                let mut trailing_whitespace_character_count = 0;
+                let text_bounds;
+                {
+                    let text = slice_chars(&*scanned_text_fragment_info.run.text,
+                                           scanned_text_fragment_info.range.begin().to_usize(),
+                                           scanned_text_fragment_info.range.end().to_usize());
+                    for ch in text.chars().rev() {
+                        if util::str::char_is_whitespace(ch) {
+                            trailing_whitespace_character_count += 1
+                        } else {
+                            break
+                        }
+                    }
+
+                    let whitespace_range =
+                        Range::new(scanned_text_fragment_info.range.end() -
+                                   CharIndex(trailing_whitespace_character_count),
+                                   CharIndex(trailing_whitespace_character_count));
+                    text_bounds = scanned_text_fragment_info.run
+                                                            .metrics_for_range(&whitespace_range)
+                                                            .bounding_box;
+                    self.border_box.size.inline = self.border_box.size.inline -
+                        text_bounds.size.width;
+                }
+
+                scanned_text_fragment_info.content_size.inline =
+                    scanned_text_fragment_info.content_size.inline - text_bounds.size.width;
+
+                if trailing_whitespace_character_count != 0 {
+                    scanned_text_fragment_info.range.extend_by(
+                        CharIndex(-trailing_whitespace_character_count));
+                }
+
+                WhitespaceStrippingResult::RetainFragment
+            }
+            SpecificFragmentInfo::UnscannedText(ref mut unscanned_text_fragment_info) => {
+                let mut trailing_bidi_control_characters_to_retain = Vec::new();
+                let (mut modified, mut last_character_index) = (true, 0);
+                for (i, character) in unscanned_text_fragment_info.text.char_indices().rev() {
+                    if gfx::text::util::is_bidi_control(character) {
+                        trailing_bidi_control_characters_to_retain.push(character);
+                        continue
+                    }
+                    if util::str::char_is_whitespace(character) {
+                        modified = true;
+                        continue
+                    }
+                    last_character_index = i + character.len_utf8();
                     break
                 }
+                if modified {
+                    let mut text = unscanned_text_fragment_info.text.to_string();
+                    text.truncate(last_character_index);
+                    for character in trailing_bidi_control_characters_to_retain.iter().rev() {
+                        text.push(*character);
+                    }
+                    unscanned_text_fragment_info.text = text.into_boxed_slice();
+                }
+
+                WhitespaceStrippingResult::from_unscanned_text_fragment_info(
+                    &unscanned_text_fragment_info)
             }
+            _ => WhitespaceStrippingResult::RetainFragment,
         }
-
-        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> {
@@ -2243,3 +2356,24 @@ impl<'a> InlineStyleIterator<'a> {
         }
     }
 }
+
+#[derive(Copy, Clone, Debug)]
+pub enum WhitespaceStrippingResult {
+    RetainFragment,
+    FragmentContainedOnlyBidiControlCharacters,
+    FragmentContainedOnlyWhitespace,
+}
+
+impl WhitespaceStrippingResult {
+    fn from_unscanned_text_fragment_info(info: &UnscannedTextFragmentInfo)
+                                         -> WhitespaceStrippingResult {
+        if info.text.is_empty() {
+            WhitespaceStrippingResult::FragmentContainedOnlyWhitespace
+        } else if info.text.chars().all(gfx::text::util::is_bidi_control) {
+            WhitespaceStrippingResult::FragmentContainedOnlyBidiControlCharacters
+        } else {
+            WhitespaceStrippingResult::RetainFragment
+        }
+    }
+}
+