Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.r?emilio draft
authorXidorn Quan <me@upsuper.org>
Thu, 24 May 2018 09:54:37 +1000 (2018-05-23)
changeset 799154 f1accf7fb9f105fef788a373af668742d78cf04f
parent 799141 4235e0be5cdf1c3d410ce28c38c8da1ad7cd74c0
push id110941
push userxquan@mozilla.com
push dateThu, 24 May 2018 01:51:15 +0000 (2018-05-24)
reviewersemilio
bugs1461285, 1415330
milestone62.0a1
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.r?emilio 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