servo: Merge #16999 - Fix stylo's text-overflow handling to match gecko (from bzbarsky:fix-text-overflow-handling); r=Manishearth
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 23 May 2017 20:39:24 -0500
changeset 408473 4ebc497ffadb87343f89f7eeb90fcb4c5fe58b9f
parent 408472 9c23bf9c0eff3730ba39c8b1cda053886b86beb1
child 408474 99b2c5908c1edc8bbbf40f08f008360d89d43669
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersManishearth
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #16999 - Fix stylo's text-overflow handling to match gecko (from bzbarsky:fix-text-overflow-handling); r=Manishearth A single value sets the text-overflow on the _end_ of the text, not both start and end. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: f3a694a7b442abad8af02475b944aeac7a09d539
servo/components/layout/inline.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/longhand/text.mako.rs
--- a/servo/components/layout/inline.rs
+++ b/servo/components/layout/inline.rs
@@ -708,17 +708,17 @@ impl LineBreaker {
             self.pending_line.range.reset(FragmentIndex(self.new_fragments.len() as isize),
                                           FragmentIndex(0));
         }
 
         // Determine if an ellipsis will be necessary to account for `text-overflow`.
         let available_inline_size = self.pending_line.green_zone.inline -
             self.pending_line.bounds.size.inline - indentation;
 
-        let ellipsis = match (&fragment.style().get_text().text_overflow.first,
+        let ellipsis = match (&fragment.style().get_text().text_overflow.second,
             fragment.style().get_box().overflow_x) {
             (&longhands::text_overflow::Side::Clip, _) | (_, overflow_x::T::visible) => None,
             (&longhands::text_overflow::Side::Ellipsis, _) => {
                 if fragment.margin_box_inline_size() > available_inline_size {
                     Some("…".to_string())
                 } else {
                     None
                 }
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -3764,38 +3764,35 @@ fn static_assert() {
                 side.mType = structs::NS_STYLE_TEXT_OVERFLOW_CLIP as u8;
             }
         }
         clear_if_string(&mut self.gecko.mTextOverflow.mLeft);
         clear_if_string(&mut self.gecko.mTextOverflow.mRight);
     }
     pub fn set_text_overflow(&mut self, v: longhands::text_overflow::computed_value::T) {
         use gecko_bindings::structs::nsStyleTextOverflowSide;
-        use properties::longhands::text_overflow::{SpecifiedValue, Side};
+        use properties::longhands::text_overflow::Side;
 
         fn set(side: &mut nsStyleTextOverflowSide, value: &Side) {
             let ty = match *value {
                 Side::Clip => structs::NS_STYLE_TEXT_OVERFLOW_CLIP,
                 Side::Ellipsis => structs::NS_STYLE_TEXT_OVERFLOW_ELLIPSIS,
                 Side::String(ref s) => {
                     side.mString.assign_utf8(s);
                     structs::NS_STYLE_TEXT_OVERFLOW_STRING
                 }
             };
             side.mType = ty as u8;
         }
 
         self.clear_overflow_sides_if_string();
-        self.gecko.mTextOverflow.mLogicalDirections = v.second.is_none();
-
-        let SpecifiedValue { ref first, ref second } = v;
-        let second = second.as_ref().unwrap_or(&first);
-
-        set(&mut self.gecko.mTextOverflow.mLeft, first);
-        set(&mut self.gecko.mTextOverflow.mRight, second);
+        self.gecko.mTextOverflow.mLogicalDirections = v.sides_are_logical;
+
+        set(&mut self.gecko.mTextOverflow.mLeft, &v.first);
+        set(&mut self.gecko.mTextOverflow.mRight, &v.second);
     }
 
     pub fn copy_text_overflow_from(&mut self, other: &Self) {
         use gecko_bindings::structs::nsStyleTextOverflowSide;
         fn set(side: &mut nsStyleTextOverflowSide, other: &nsStyleTextOverflowSide) {
             if other.mType == structs::NS_STYLE_TEXT_OVERFLOW_STRING as u8 {
                 side.mString.assign(&*other.mString)
             }
--- a/servo/components/style/properties/longhand/text.mako.rs
+++ b/servo/components/style/properties/longhand/text.mako.rs
@@ -11,20 +11,18 @@
                          additional_methods=[Method("has_underline", "bool"),
                                              Method("has_overline", "bool"),
                                              Method("has_line_through", "bool")]) %>
 
 <%helpers:longhand name="text-overflow" animation_value_type="none" boxed="True"
                    spec="https://drafts.csswg.org/css-ui/#propdef-text-overflow">
     use std::fmt;
     use style_traits::ToCss;
-    use values::computed::ComputedValueAsSpecified;
     use cssparser;
 
-    impl ComputedValueAsSpecified for SpecifiedValue {}
     no_viewport_percentage!(SpecifiedValue);
 
     #[derive(PartialEq, Eq, Clone, Debug)]
     #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
     pub enum Side {
         Clip,
         Ellipsis,
         String(Box<str>),
@@ -33,24 +31,83 @@
     #[derive(PartialEq, Eq, Clone, Debug)]
     #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
     pub struct SpecifiedValue {
         pub first: Side,
         pub second: Option<Side>
     }
 
     pub mod computed_value {
-        pub type T = super::SpecifiedValue;
+        pub use super::Side;
+
+        #[derive(Debug, Clone, PartialEq)]
+        #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+        pub struct T {
+            // When the specified value only has one side, that's the "second"
+            // side, and the sides are logical, so "second" means "end".  The
+            // start side is Clip in that case.
+            //
+            // When the specified value has two sides, those are our "first"
+            // and "second" sides, and they are physical sides ("left" and
+            // "right").
+            pub first: Side,
+            pub second: Side,
+            pub sides_are_logical: bool
+        }
+    }
+
+    impl ToCss for computed_value::T {
+        fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+            if self.sides_are_logical {
+                assert!(self.first == Side::Clip);
+                try!(self.second.to_css(dest));
+            } else {
+                try!(self.first.to_css(dest));
+                try!(dest.write_str(" "));
+                try!(self.second.to_css(dest));
+            }
+            Ok(())
+        }
+    }
+
+    impl ToComputedValue for SpecifiedValue {
+        type ComputedValue = computed_value::T;
+
+        #[inline]
+        fn to_computed_value(&self, _context: &Context) -> Self::ComputedValue {
+            if let Some(ref second) = self.second {
+                Self::ComputedValue { first: self.first.clone(),
+                                      second: second.clone(),
+                                      sides_are_logical: false }
+            } else {
+                Self::ComputedValue { first: Side::Clip,
+                                      second: self.first.clone(),
+                                      sides_are_logical: true }
+            }
+        }
+
+        #[inline]
+        fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+            if computed.sides_are_logical {
+                assert!(computed.first == Side::Clip);
+                SpecifiedValue { first: computed.second.clone(),
+                                 second: None }
+            } else {
+                SpecifiedValue { first: computed.first.clone(),
+                                 second: Some(computed.second.clone()) }
+            }
+        }
     }
 
     #[inline]
     pub fn get_initial_value() -> computed_value::T {
-        SpecifiedValue {
+        computed_value::T {
             first: Side::Clip,
-            second: None
+            second: Side::Clip,
+            sides_are_logical: true,
         }
     }
     pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
         let first = try!(Side::parse(context, input));
         let second = input.try(|input| Side::parse(context, input)).ok();
         Ok(SpecifiedValue {
             first: first,
             second: second,