Implement the unicode-bidi property

components/gfx/font.rs

@@ -146,7 +146,8 @@ impl Font {
         }
 
         let mut glyphs = GlyphStore::new(text.chars().count(),
-                                         options.flags.contains(IS_WHITESPACE_SHAPING_FLAG));
+                                         options.flags.contains(IS_WHITESPACE_SHAPING_FLAG),
+                                         options.flags.contains(RTL_FLAG));
         shaper.as_ref().unwrap().shape_text(text, options, &mut glyphs);
 
         let glyphs = Arc::new(glyphs);

components/gfx/text/glyph.rs

@@ -512,6 +512,7 @@ pub struct GlyphStore {
     detail_store: DetailedGlyphStore,
 
     is_whitespace: bool,
+    is_rtl: bool,
 }
 
 int_range_index! {
@@ -526,13 +527,14 @@ impl<'a> GlyphStore {
     /// Initializes the glyph store, but doesn't actually shape anything.
     ///
     /// Use the `add_*` methods to store glyph data.
-    pub fn new(length: usize, is_whitespace: bool) -> GlyphStore {
+    pub fn new(length: usize, is_whitespace: bool, is_rtl: bool) -> GlyphStore {
         assert!(length > 0);
 
         GlyphStore {
             entry_buffer: vec![GlyphEntry::initial(); length],
             detail_store: DetailedGlyphStore::new(),
             is_whitespace: is_whitespace,
+            is_rtl: is_rtl,
         }
     }
 
@@ -633,8 +635,8 @@ impl<'a> GlyphStore {
 
         GlyphIterator {
             store:       self,
-            char_index:  rang.begin(),
+            char_index:  if self.is_rtl { rang.end() } else { rang.begin() - CharIndex(1) },
-            char_range:  rang.each_index(),
+            char_range:  *rang,
             glyph_range: None,
         }
     }
@@ -736,7 +738,7 @@ impl<'a> GlyphStore {
 pub struct GlyphIterator<'a> {
     store: &'a GlyphStore,
     char_index: CharIndex,
-    char_range: EachIndex<isize, CharIndex>,
+    char_range: Range<CharIndex>,
     glyph_range: Option<EachIndex<isize, CharIndex>>,
 }
 
@@ -776,23 +778,28 @@ impl<'a> Iterator for GlyphIterator<'a> {
     // `next_complex_glyph()`.
     #[inline(always)]
     fn next(&mut self) -> Option<(CharIndex, GlyphInfo<'a>)> {
-        // Would use 'match' here but it borrows contents in a way that
+        // Would use 'match' here but it borrows contents in a way that interferes with mutation.
-        // interferes with mutation.
         if self.glyph_range.is_some() {
-            self.next_glyph_range()
+            return self.next_glyph_range()
+        }
+
+        // No glyph range. Look at next character.
+        self.char_index = self.char_index + if self.store.is_rtl {
+            CharIndex(-1)
         } else {
-            // No glyph range. Look at next character.
+            CharIndex(1)
-            self.char_range.next().and_then(|i| {
+        };
-                self.char_index = i;
+        let i = self.char_index;
-                assert!(i < self.store.char_len());
+        if !self.char_range.contains(i) {
-                let entry = self.store.entry_buffer[i.to_usize()];
+            return None
-                if entry.is_simple() {
+        }
-                    Some((self.char_index, GlyphInfo::Simple(self.store, i)))
+        debug_assert!(i < self.store.char_len());
-                } else {
+        let entry = self.store.entry_buffer[i.to_usize()];
-                    // Fall back to the slow path.
+        if entry.is_simple() {
-                    self.next_complex_glyph(&entry, i)
+            Some((i, GlyphInfo::Simple(self.store, i)))
-                }
+        } else {
-            })
+            // Fall back to the slow path.
+            self.next_complex_glyph(&entry, i)
         }
     }
 }

components/gfx/text/shaping/harfbuzz.rs

@@ -9,7 +9,7 @@ use font::{IGNORE_LIGATURES_SHAPING_FLAG, RTL_FLAG, ShapingOptions};
 use platform::font::FontTable;
 use text::glyph::{CharIndex, GlyphStore, GlyphId, GlyphData};
 use text::shaping::ShaperMethods;
-use text::util::{float_to_fixed, fixed_to_float};
+use text::util::{float_to_fixed, fixed_to_float, is_bidi_control};
 
 use euclid::Point2D;
 use harfbuzz::{HB_MEMORY_MODE_READONLY, HB_DIRECTION_LTR, HB_DIRECTION_RTL};
@@ -279,7 +279,11 @@ impl Shaper {
 
         // GlyphStore records are indexed by character, not byte offset.
         // so, we must be careful to increment this when saving glyph entries.
-        let mut char_idx = CharIndex(0);
+        let (mut char_idx, char_step) = if options.flags.contains(RTL_FLAG) {
+            (CharIndex(char_max as isize - 1), CharIndex(-1))
+        } else {
+            (CharIndex(0), CharIndex(1))
+        };
 
         debug!("Shaped text[char count={}], got back {} glyph info records.",
                char_max,
@@ -318,10 +322,10 @@ impl Shaper {
             debug!("{} -> {}", i, loc);
         }
 
-        debug!("text: {}", text);
+        debug!("text: {:?}", text);
         debug!("(char idx): char->(glyph index):");
         for (i, ch) in text.char_indices() {
-            debug!("{}: {} --> {}", i, ch, byte_to_glyph[i]);
+            debug!("{}: {:?} --> {}", i, ch, byte_to_glyph[i]);
         }
 
         // some helpers
@@ -453,16 +457,20 @@ impl Shaper {
                 //
                 // NB: When we acquire the ability to handle ligatures that cross word boundaries,
                 // we'll need to do something special to handle `word-spacing` properly.
-                let shape = glyph_data.get_entry_for_glyph(glyph_span.begin(), &mut y_pos);
                 let character = text.char_at(char_byte_span.begin());
-                let advance = self.advance_for_shaped_glyph(shape.advance, character, options);
+                if is_bidi_control(character) {
-                let data = GlyphData::new(shape.codepoint,
+                    glyphs.add_nonglyph_for_char_index(char_idx, false, false);
-                                          advance,
+                } else {
-                                          shape.offset,
+                    let shape = glyph_data.get_entry_for_glyph(glyph_span.begin(), &mut y_pos);
-                                          false,
+                    let advance = self.advance_for_shaped_glyph(shape.advance, character, options);
-                                          true,
+                    let data = GlyphData::new(shape.codepoint,
-                                          true);
+                                              advance,
-                glyphs.add_glyph_for_char_index(char_idx, Some(character), &data);
+                                              shape.offset,
+                                              false,
+                                              true,
+                                              true);
+                    glyphs.add_glyph_for_char_index(char_idx, Some(character), &data);
+                }
             } else {
                 // collect all glyphs to be assigned to the first character.
                 let mut datas = vec!();
@@ -488,7 +496,7 @@ impl Shaper {
                     drop(range.ch);
                     i = range.next;
                     if i >= covered_byte_span.end() { break; }
-                    char_idx = char_idx + CharIndex(1);
+                    char_idx = char_idx + char_step;
                     glyphs.add_nonglyph_for_char_index(char_idx, false, false);
                 }
             }
@@ -498,7 +506,7 @@ impl Shaper {
             glyph_span.reset(end, 0);
             let end = char_byte_span.end();; // FIXME: borrow checker workaround
             char_byte_span.reset(end, 0);
-            char_idx = char_idx + CharIndex(1);
+            char_idx = char_idx + char_step;
         }
 
         // this must be called after adding all glyph data; it sorts the

components/gfx/text/util.rs

@@ -92,9 +92,9 @@ pub fn transform_text(text: &str,
         }
     }
 
-    fn is_always_discardable_char(_ch: char) -> bool {
+    fn is_always_discardable_char(ch: char) -> bool {
-        // TODO: check for bidi control chars, soft hyphens.
+        // TODO: check for soft hyphens.
-        false
+        is_bidi_control(ch)
     }
 }
 
@@ -105,3 +105,12 @@ pub fn float_to_fixed(before: usize, f: f64) -> i32 {
 pub fn fixed_to_float(before: usize, f: i32) -> f64 {
     f as f64 * 1.0f64 / ((1i32 << before) as f64)
 }
+
+pub fn is_bidi_control(c: char) -> bool {
+    match c {
+        '\u{202A}'...'\u{202E}' => true,
+        '\u{2066}'...'\u{2069}' => true,
+        '\u{200E}' | '\u{200F}' | '\u{061C}' => true,
+        _ => false
+    }
+}

components/layout/construct.rs

@@ -194,6 +194,12 @@ struct InlineFragmentsAccumulator {
     /// Whether we've created a range to enclose all the fragments. This will be Some() if the
     /// outer node is an inline and None otherwise.
     enclosing_node: Option<InlineFragmentNodeInfo>,
+
+    /// Restyle damage to use for fragments created in this node.
+    restyle_damage: RestyleDamage,
+
+    /// Bidi control characters to insert before and after these fragments.
+    bidi_control_chars: Option<(&'static str, &'static str)>,
 }
 
 impl InlineFragmentsAccumulator {
@@ -201,6 +207,8 @@ impl InlineFragmentsAccumulator {
         InlineFragmentsAccumulator {
             fragments: IntermediateInlineFragments::new(),
             enclosing_node: None,
+            bidi_control_chars: None,
+            restyle_damage: RestyleDamage::empty(),
         }
     }
 
@@ -211,6 +219,8 @@ impl InlineFragmentsAccumulator {
                 address: node.opaque(),
                 style: node.style().clone(),
             }),
+            bidi_control_chars: None,
+            restyle_damage: node.restyle_damage(),
         }
     }
 
@@ -227,6 +237,8 @@ impl InlineFragmentsAccumulator {
         let InlineFragmentsAccumulator {
             mut fragments,
             enclosing_node,
+            bidi_control_chars,
+            restyle_damage,
         } = self;
         if let Some(enclosing_node) = enclosing_node {
             let frag_len = fragments.fragments.len();
@@ -239,6 +251,15 @@ impl InlineFragmentsAccumulator {
 
                 frag.add_inline_context_style(enclosing_node.clone(), is_first, is_last);
             }
+
+            // Control characters are later discarded in transform_text, so they don't affect the
+            // is_first/is_last styles above.
+            if let Some((start, end)) = bidi_control_chars {
+                fragments.fragments.push_front(
+                    control_chars_to_fragment(&enclosing_node, start, restyle_damage));
+                fragments.fragments.push_back(
+                    control_chars_to_fragment(&enclosing_node, end, restyle_damage));
+            }
         }
         fragments
     }
@@ -744,6 +765,8 @@ impl<'a> FlowConstructor<'a> {
                                                       -> ConstructionResult {
         let mut opt_inline_block_splits: LinkedList<InlineBlockSplit> = LinkedList::new();
         let mut fragment_accumulator = InlineFragmentsAccumulator::from_inline_node(node);
+        fragment_accumulator.bidi_control_chars = bidi_control_chars(&*node.style());
+
         let mut abs_descendants = Descendants::new();
 
         // Concatenate all the fragments of our kids, creating {ib} splits as necessary.
@@ -1631,3 +1654,37 @@ pub fn strip_ignorable_whitespace_from_end(this: &mut LinkedList<Fragment>) {
         drop(this.pop_back());
     }
 }
+
+/// If the 'unicode-bidi' property has a value other than 'normal', return the bidi control codes
+/// to inject before and after the text content of the element.
+fn bidi_control_chars(style: &Arc<ComputedValues>) -> Option<(&'static str, &'static str)> {
+    use style::computed_values::unicode_bidi::T::*;
+    use style::computed_values::direction::T::*;
+
+    let unicode_bidi = style.get_text().unicode_bidi;
+    let direction = style.get_inheritedbox().direction;
+
+    // See the table in http://dev.w3.org/csswg/css-writing-modes/#unicode-bidi
+    match (unicode_bidi, direction) {
+        (normal, _)             => None,
+        (embed, ltr)            => Some(("\u{202A}", "\u{202C}")),
+        (embed, rtl)            => Some(("\u{202B}", "\u{202C}")),
+        (isolate, ltr)          => Some(("\u{2066}", "\u{2069}")),
+        (isolate, rtl)          => Some(("\u{2067}", "\u{2069}")),
+        (bidi_override, ltr)    => Some(("\u{202D}", "\u{202C}")),
+        (bidi_override, rtl)    => Some(("\u{202E}", "\u{202C}")),
+        (isolate_override, ltr) => Some(("\u{2068}\u{202D}", "\u{202C}\u{2069}")),
+        (isolate_override, rtl) => Some(("\u{2068}\u{202E}", "\u{202C}\u{2069}")),
+        (plaintext, _)          => Some(("\u{2068}", "\u{2069}")),
+    }
+}
+
+fn control_chars_to_fragment(node: &InlineFragmentNodeInfo, text: &str,
+                             restyle_damage: RestyleDamage) -> Fragment {
+    let info = SpecificFragmentInfo::UnscannedText(
+        UnscannedTextFragmentInfo::from_text(String::from(text)));
+    Fragment::from_opaque_node_and_style(node.address,
+                                         node.style.clone(),
+                                         restyle_damage,
+                                         info)
+}

components/layout/fragment.rs

@@ -1621,7 +1621,7 @@ impl Fragment {
         }
         match self.specific {
             SpecificFragmentInfo::UnscannedText(ref text_fragment_info) => {
-                util::str::is_whitespace(&text_fragment_info.text)
+                is_whitespace(&text_fragment_info.text)
             }
             _ => false,
         }

components/script/dom/webidls/CSSStyleDeclaration.webidl

@@ -99,6 +99,7 @@ partial interface CSSStyleDeclaration {
   [TreatNullAs=EmptyString] attribute DOMString backfaceVisibility;
 
   [TreatNullAs=EmptyString] attribute DOMString direction;
+  [TreatNullAs=EmptyString] attribute DOMString unicodeBidi;
 
   [TreatNullAs=EmptyString] attribute DOMString filter;
 

components/style/properties.mako.rs

@@ -2076,6 +2076,8 @@ pub mod longhands {
 
     ${new_style_struct("Text", is_inherited=False)}
 
+    ${single_keyword("unicode-bidi", "normal embed isolate bidi-override isolate-override plaintext")}
+
     <%self:longhand name="text-decoration" custom_cascade="True">
         use cssparser::ToCss;
         use std::fmt;

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

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

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

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

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

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

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

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

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

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

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

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

tests/wpt/metadata-css/css21_dev/html4/bidi-box-model-006.htm.ini

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

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

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

tests/wpt/metadata-css/css21_dev/html4/inline-formatting-context-007.htm.ini

@@ -1,3 +1,4 @@
 [inline-formatting-context-007.htm]
   type: reftest
-  expected: FAIL
+  expected:
+    if os == "mac": FAIL

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

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

tests/wpt/metadata/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-nested.html.ini

@@ -2,4 +2,5 @@
   type: reftest
   reftype: ==
   refurl: /html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-nested-ref.html
-  expected: FAIL
+  expected:
+    if os != "mac": FAIL

tests/wpt/metadata/html/semantics/text-level-semantics/the-bdo-element/bdo-override.html.ini

@@ -1,3 +0,0 @@
-[bdo-override.html]
-  type: reftest
-  expected: FAIL