servo: Merge #18352 - Make serialization match Gecko in a few corner cases (from jdm:serialize-fun); r=emilio
authorJosh Matthews <josh@joshmatthews.net>
Wed, 13 Sep 2017 10:51:00 -0500
changeset 430114 db9ef4f3705b3a4541fef5093398ba43c110fc15
parent 430113 763af7d6686da5cabb27188dafd70d9ef07ce9f0
child 430115 6c8fd74d8dcf326d0f2c96d45847312ad2725ce6
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs18352, 1345218
milestone57.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 #18352 - Make serialization match Gecko in a few corner cases (from jdm:serialize-fun); r=emilio This addresses the testcases from https://bugzilla.mozilla.org/show_bug.cgi?id=1345218. Gecko differs from the specification by doing the following: * reusing longhands that have previously been serialized in order to serialize shorthands * immediately breaking out of the shorthand loop for the current property as soon as a shorthand is successfully serialized https://github.com/w3c/csswg-drafts/issues/1774 is filed to track ways that the standard could be made more clear on these points. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [bug 1345218](https://bugzilla.mozilla.org/show_bug.cgi?id=1345218). - [X] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: e50341d4a91beded42bdcdf37bfb8a7e53070234
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
servo/tests/unit/style/properties/serialization.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -652,36 +652,28 @@ impl ToCss for PropertyDeclarationBlock 
                         shorthand == ShorthandId::Font &&
                         self.declarations.iter().any(|l| {
                             !already_serialized.contains(l.id()) &&
                             l.get_system().is_some()
                         });
 
                     if is_system_font {
                         for (longhand, importance) in self.declaration_importance_iter() {
-                            if already_serialized.contains(longhand.id()) {
-                                continue;
-                            }
-
                             if longhand.get_system().is_some() || longhand.is_default_line_height() {
                                 current_longhands.push(longhand);
                                 if found_system.is_none() {
                                    found_system = longhand.get_system();
                                 }
                                 if importance.important() {
                                     important_count += 1;
                                 }
                             }
                         }
                     } else {
                         for (longhand, importance) in self.declaration_importance_iter() {
-                            if already_serialized.contains(longhand.id()) {
-                                continue;
-                            }
-
                             if longhand.id().is_longhand_of(shorthand) {
                                 current_longhands.push(longhand);
                                 if importance.important() {
                                     important_count += 1;
                                 }
                             }
                         }
                         // Substep 1:
@@ -766,16 +758,23 @@ impl ToCss for PropertyDeclarationBlock 
                              importance,
                              &mut is_first_serialization)?;
                     }
 
                     for current_longhand in &current_longhands {
                         // Substep 9
                         already_serialized.insert(current_longhand.id());
                     }
+
+                    // FIXME(https://github.com/w3c/csswg-drafts/issues/1774)
+                    // The specification does not include an instruction to abort
+                    // the shorthand loop at this point, but doing so both matches
+                    // Gecko and makes sense since shorthands are checked in
+                    // preferred order.
+                    break;
                 }
             }
 
             // Step 3.3.4
             if already_serialized.contains(property) {
                 continue;
             }
 
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -471,25 +471,38 @@ impl LonghandId {
     fn shorthands(&self) -> &'static [ShorthandId] {
         // first generate longhand to shorthands lookup map
         //
         // NOTE(emilio): This currently doesn't exclude the "all" shorthand. It
         // could potentially do so, which would speed up serialization
         // algorithms and what not, I guess.
         <%
             longhand_to_shorthand_map = {}
+            num_sub_properties = {}
             for shorthand in data.shorthands:
+                num_sub_properties[shorthand.camel_case] = len(shorthand.sub_properties)
                 for sub_property in shorthand.sub_properties:
                     if sub_property.ident not in longhand_to_shorthand_map:
                         longhand_to_shorthand_map[sub_property.ident] = []
 
                     longhand_to_shorthand_map[sub_property.ident].append(shorthand.camel_case)
 
+            def preferred_order(x, y):
+                # Since we want properties in order from most subproperties to least,
+                # reverse the arguments to cmp from the expected order.
+                result = cmp(num_sub_properties.get(y, 0), num_sub_properties.get(x, 0))
+                if result:
+                    return result
+                # Fall back to lexicographic comparison.
+                return cmp(x, y)
+
+            # Sort the lists of shorthand properties according to preferred order:
+            # https://drafts.csswg.org/cssom/#concept-shorthands-preferred-order
             for shorthand_list in longhand_to_shorthand_map.itervalues():
-                shorthand_list.sort()
+                shorthand_list.sort(cmp=preferred_order)
         %>
 
         // based on lookup results for each longhand, create result arrays
         % for property in data.longhands:
             static ${property.ident.upper()}: &'static [ShorthandId] = &[
                 % for shorthand in longhand_to_shorthand_map.get(property.ident, []):
                     ShorthandId::${shorthand},
                 % endfor
--- a/servo/tests/unit/style/properties/serialization.rs
+++ b/servo/tests/unit/style/properties/serialization.rs
@@ -336,16 +336,63 @@ mod shorthand_serialization {
             assert_eq!(serialization, "border-radius: 1% 2% 3% 4% / 5% 6% 7% 8%;");
         }
     }
 
 
     mod border_shorthands {
         use super::*;
 
+        #[test]
+        fn border_top_and_color() {
+            let mut properties = Vec::new();
+            properties.push(PropertyDeclaration::BorderTopWidth(BorderSideWidth::Length(Length::from_px(1.))));
+            properties.push(PropertyDeclaration::BorderTopStyle(BorderStyle::solid));
+            let c = Color::Numeric {
+                parsed: RGBA::new(255, 0, 0, 255),
+                authored: Some("green".to_string().into_boxed_str())
+            };
+            properties.push(PropertyDeclaration::BorderTopColor(c));
+            let c = Color::Numeric {
+                parsed: RGBA::new(0, 255, 0, 255),
+                authored: Some("red".to_string().into_boxed_str())
+            };
+            properties.push(PropertyDeclaration::BorderTopColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderBottomColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderLeftColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderRightColor(c.clone()));
+
+            let serialization = shorthand_properties_to_string(properties);
+            assert_eq!(serialization, "border-top: 1px solid red; border-color: red;");
+        }
+
+        #[test]
+        fn border_color_and_top() {
+            let mut properties = Vec::new();
+                let c = Color::Numeric {
+                parsed: RGBA::new(0, 255, 0, 255),
+                authored: Some("red".to_string().into_boxed_str())
+            };
+            properties.push(PropertyDeclaration::BorderTopColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderBottomColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderLeftColor(c.clone()));
+            properties.push(PropertyDeclaration::BorderRightColor(c.clone()));
+
+            properties.push(PropertyDeclaration::BorderTopWidth(BorderSideWidth::Length(Length::from_px(1.))));
+            properties.push(PropertyDeclaration::BorderTopStyle(BorderStyle::solid));
+            let c = Color::Numeric {
+                parsed: RGBA::new(255, 0, 0, 255),
+                authored: Some("green".to_string().into_boxed_str())
+            };
+            properties.push(PropertyDeclaration::BorderTopColor(c));
+
+            let serialization = shorthand_properties_to_string(properties);
+            assert_eq!(serialization, "border-color: green red red; border-top: 1px solid green;");
+        }
+
         // we can use border-top as a base to test out the different combinations
         // but afterwards, we only need to to one test per "directional border shorthand"
 
         #[test]
         fn directional_border_should_show_all_properties_when_values_are_set() {
             let mut properties = Vec::new();
 
             let width = BorderSideWidth::Length(Length::from_px(4f32));