Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event. r=emilio, a=RyanVM
authorXidorn Quan <me@upsuper.org>
Thu, 24 May 2018 09:54:37 +1000
changeset 470887 88c81330970bd1f7336157648668366af72e57bc
parent 470886 a1ae29c3826c5d1ea323648667e19ac1b541c13f
child 470888 4fa86932be2d7a90b2494de9ae6c379c86a031f9
push id9256
push userryanvm@gmail.com
push dateThu, 24 May 2018 15:32:42 +0000
treeherdermozilla-beta@54ba8ee1c9ce [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio, RyanVM
bugs1461285, 1415330
milestone61.0
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event. r=emilio, a=RyanVM MozReview-Commit-ID: HesaR9prhvy
servo/components/style/properties/declaration_block.rs
testing/web-platform/meta/css/cssom/cssstyledeclaration-setter-order.html.ini
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -513,43 +513,63 @@ impl PropertyDeclarationBlock {
         if !definitely_new {
             let mut index_to_remove = None;
             for (i, slot) in self.declarations.iter_mut().enumerate() {
                 if slot.id() != declaration.id() {
                     continue;
                 }
 
                 let important = self.declarations_importance[i];
-                // For declarations from parsing, non-important declarations
-                // shouldn't override existing important one.
-                if important && !importance.important() &&
-                    matches!(source, DeclarationSource::Parsing) {
-                    return true;
-                }
+                match (important, importance.important()) {
+                    (false, true) => {}
 
-                if matches!(source, DeclarationSource::Parsing) {
-                    // As a compatibility hack, specially on Android,
-                    // don't allow to override a prefixed webkit display
-                    // value with an unprefixed version from parsing
-                    // code.
-                    //
-                    // TODO(emilio): Unship.
-                    if let PropertyDeclaration::Display(old_display) = *slot {
-                        use properties::longhands::display::computed_value::T as display;
-
-                        if let PropertyDeclaration::Display(new_display) = declaration {
-                            if display::should_ignore_parsed_value(old_display, new_display) {
-                                return false;
-                            }
+                    (true, false) => {
+                        // For declarations set from the OM, more-important
+                        // declarations are overridden.
+                        if !matches!(source, DeclarationSource::CssOm) {
+                            return false
                         }
                     }
+                    _ => if *slot == declaration {
+                        return false;
+                    }
                 }
 
-                index_to_remove = Some(i);
-                break;
+                match source {
+                    // CSSOM preserves the declaration position, and
+                    // overrides importance.
+                    DeclarationSource::CssOm => {
+                        *slot = declaration;
+                        self.declarations_importance.set(i, importance.important());
+                        return true;
+                    }
+                    DeclarationSource::Parsing => {
+                        // As a compatibility hack, specially on Android,
+                        // don't allow to override a prefixed webkit display
+                        // value with an unprefixed version from parsing
+                        // code.
+                        //
+                        // TODO(emilio): Unship.
+                        if let PropertyDeclaration::Display(old_display) = *slot {
+                            use properties::longhands::display::computed_value::T as display;
+
+                            if let PropertyDeclaration::Display(new_display) = declaration {
+                                if display::should_ignore_parsed_value(old_display, new_display) {
+                                    return false;
+                                }
+                            }
+                        }
+
+                        // NOTE(emilio): We could avoid this and just override for
+                        // properties not affected by logical props, but it's not
+                        // clear it's worth it given the `definitely_new` check.
+                        index_to_remove = Some(i);
+                        break;
+                    }
+                }
             }
 
             if let Some(index) = index_to_remove {
                 self.declarations.remove(index);
                 self.declarations_importance.remove(index);
                 self.declarations.push(declaration);
                 self.declarations_importance.push(importance.important());
                 return true;
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/css/cssom/cssstyledeclaration-setter-order.html.ini
@@ -0,0 +1,12 @@
+[cssstyledeclaration-setter-order.html]
+  [setProperty with existing longhand should change order]
+    expected: FAIL
+
+  [invoke property setter with existing longhand should change order]
+    expected: FAIL
+
+  [setProperty with existing shorthand should change order]
+    expected: FAIL
+
+  [invoke property setter with existing shorthand should change order]
+    expected: FAIL