Auto merge of #18867 - emilio:parse-hack, r=heycam

style: Dishonor display: -moz-box if -webkit-box was specified before

This is a compatibility hack that Gecko supports that is apparently important for android.

I want to remove it, but let's see...

See https://bugzilla.mozilla.org/show_bug.cgi?id=1407701 for reference.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18867)
<!-- Reviewable:end -->

components/script/dom/cssstyledeclaration.rs

@@ -18,7 +18,7 @@ use servo_arc::Arc;
 use servo_url::ServoUrl;
 use std::ascii::AsciiExt;
 use style::attr::AttrValue;
-use style::properties::{Importance, PropertyDeclarationBlock, PropertyId, LonghandId, ShorthandId};
+use style::properties::{DeclarationSource, Importance, PropertyDeclarationBlock, PropertyId, LonghandId, ShorthandId};
 use style::properties::{parse_one_declaration_into, parse_style_attribute, SourcePropertyDeclaration};
 use style::selector_parser::PseudoElement;
 use style::shared_lock::Locked;
@@ -274,7 +274,11 @@ impl CSSStyleDeclaration {
 
             // Step 7
             // Step 8
-            *changed = pdb.extend_reset(declarations.drain(), importance);
+            *changed = pdb.extend(
+                declarations.drain(),
+                importance,
+                DeclarationSource::CssOm,
+            );
 
             Ok(())
         })

components/style/properties/declaration_block.rs

@@ -39,6 +39,17 @@ impl AnimationRules {
     }
 }
 
+/// Whether a given declaration comes from CSS parsing, or from CSSOM.
+#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+pub enum DeclarationSource {
+    /// The declaration was obtained from CSS parsing of sheets and such.
+    Parsing,
+    /// The declaration was obtained from CSSOM.
+    CssOm,
+}
+
 /// A declaration [importance][importance].
 ///
 /// [importance]: https://drafts.csswg.org/css-cascade/#importance
@@ -378,30 +389,15 @@ impl PropertyDeclarationBlock {
         }
     }
 
-    /// Adds or overrides the declaration for a given property in this block,
-    /// **except** if an existing declaration for the same property is more
-    /// important.
+    /// Adds or overrides the declaration for a given property in this block.
     ///
-    /// Always ensures that the property declaration is at the end.
-    pub fn extend(&mut self, drain: SourcePropertyDeclarationDrain, importance: Importance) {
-        self.extend_common(drain, importance, false);
-    }
-
-    /// Adds or overrides the declaration for a given property in this block,
-    /// **even** if an existing declaration for the same property is more
-    /// important, and reuses the same position in the block.
-    ///
-    /// Returns whether anything changed.
-    pub fn extend_reset(&mut self, drain: SourcePropertyDeclarationDrain,
-                        importance: Importance) -> bool {
-        self.extend_common(drain, importance, true)
-    }
-
-    fn extend_common(
+    /// See the documentation of `push` to see what impact `source` has when the
+    /// property is already there.
+    pub fn extend(
         &mut self,
         mut drain: SourcePropertyDeclarationDrain,
         importance: Importance,
-        overwrite_more_important_and_reuse_slot: bool,
+        source: DeclarationSource,
     ) -> bool {
         let all_shorthand_len = match drain.all_shorthand {
             AllShorthand::NotSet => 0,
@@ -415,10 +411,10 @@ impl PropertyDeclarationBlock {
 
         let mut changed = false;
         for decl in &mut drain.declarations {
-            changed |= self.push_common(
+            changed |= self.push(
                 decl,
                 importance,
-                overwrite_more_important_and_reuse_slot,
+                source,
             );
         }
         match drain.all_shorthand {
@@ -426,20 +422,20 @@ impl PropertyDeclarationBlock {
             AllShorthand::CSSWideKeyword(keyword) => {
                 for &id in ShorthandId::All.longhands() {
                     let decl = PropertyDeclaration::CSSWideKeyword(id, keyword);
-                    changed |= self.push_common(
+                    changed |= self.push(
                         decl,
                         importance,
-                        overwrite_more_important_and_reuse_slot,
+                        source,
                     );
                 }
             }
             AllShorthand::WithVariables(unparsed) => {
                 for &id in ShorthandId::All.longhands() {
                     let decl = PropertyDeclaration::WithVariables(id, unparsed.clone());
-                    changed |= self.push_common(
+                    changed |= self.push(
                         decl,
                         importance,
-                        overwrite_more_important_and_reuse_slot,
+                        source,
                     );
                 }
             }
@@ -447,21 +443,24 @@ impl PropertyDeclarationBlock {
         changed
     }
 
-    /// Adds or overrides the declaration for a given property in this block,
-    /// **except** if an existing declaration for the same property is more
-    /// important.
+    /// Adds or overrides the declaration for a given property in this block.
     ///
-    /// Ensures that, if inserted, it's inserted at the end of the declaration
+    /// Depending on the value of `source`, this has a different behavior in the
+    /// presence of another declaration with the same ID in the declaration
     /// block.
-    pub fn push(&mut self, declaration: PropertyDeclaration, importance: Importance) {
-        self.push_common(declaration, importance, false);
-    }
-
-    fn push_common(
+    ///
+    ///   * For `DeclarationSource::Parsing`, this will not override a
+    ///     declaration with more importance, and will ensure that, if inserted,
+    ///     it's inserted at the end of the declaration block.
+    ///
+    ///   * For `DeclarationSource::CssOm`, this will override importance and
+    ///     will preserve the original position on the block.
+    ///
+    pub fn push(
         &mut self,
         declaration: PropertyDeclaration,
         importance: Importance,
-        overwrite_more_important_and_reuse_slot: bool
+        source: DeclarationSource,
     ) -> bool {
         let longhand_id = match declaration.id() {
             PropertyDeclarationId::Longhand(id) => Some(id),
@@ -481,7 +480,9 @@ impl PropertyDeclarationBlock {
                         (false, true) => {}
 
                         (true, false) => {
-                            if !overwrite_more_important_and_reuse_slot {
+                            // For declarations set from the OM, less-important
+                            // declarations are overridden.
+                            if !matches!(source, DeclarationSource::CssOm) {
                                 return false
                             }
                         }
@@ -490,17 +491,41 @@ impl PropertyDeclarationBlock {
                         }
                     }
 
-                    if overwrite_more_important_and_reuse_slot {
-                        *slot = declaration;
-                        self.declarations_importance.set(i as u32, importance.important());
-                        return true;
-                    }
+                    match source {
+                        // CSSOM preserves the declaration position, and
+                        // overrides importance.
+                        DeclarationSource::CssOm => {
+                            *slot = declaration;
+                            self.declarations_importance.set(i as u32, 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;
 
-                    // 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;
+                                let new_display = match declaration {
+                                    PropertyDeclaration::Display(new_display) => new_display,
+                                    _ => unreachable!("How could the declaration id be the same?"),
+                                };
+
+                                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;
+                        }
+                    }
                 }
             }
 
@@ -1122,7 +1147,11 @@ where
     while let Some(declaration) = iter.next() {
         match declaration {
             Ok(importance) => {
-                block.extend(iter.parser.declarations.drain(), importance);
+                block.extend(
+                    iter.parser.declarations.drain(),
+                    importance,
+                    DeclarationSource::Parsing,
+                );
             }
             Err((error, slice)) => {
                 iter.parser.declarations.clear();

components/style/properties/longhand/box.mako.rs

@@ -68,6 +68,33 @@
                 )
             }
 
+            /// Whether `new_display` should be ignored, given a previous
+            /// `old_display` value.
+            ///
+            /// This is used to ignore `display: -moz-box` declarations after an
+            /// equivalent `display: -webkit-box` declaration, since the former
+            /// has a vastly different meaning. See bug 1107378 and bug 1407701.
+            ///
+            /// FIXME(emilio): This is a pretty decent hack, we should try to
+            /// remove it.
+            pub fn should_ignore_parsed_value(
+                _old_display: Self,
+                _new_display: Self,
+            ) -> bool {
+                #[cfg(feature = "gecko")]
+                {
+                    match (_old_display, _new_display) {
+                        (T::_webkit_box, T::_moz_box) |
+                        (T::_webkit_inline_box, T::_moz_inline_box) => {
+                            return true;
+                        }
+                        _ => {},
+                    }
+                }
+
+                return false;
+            }
+
             /// Returns whether this "display" value is one of the types for
             /// ruby.
             #[cfg(feature = "gecko")]

components/style/stylesheets/keyframes_rule.rs

@@ -8,8 +8,8 @@ use cssparser::{AtRuleParser, Parser, QualifiedRuleParser, RuleListParser, Parse
 use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule, SourceLocation};
 use error_reporting::{NullReporter, ContextualParseError, ParseErrorReporter};
 use parser::{ParserContext, ParserErrorContext};
-use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId, PropertyParserContext};
-use properties::{PropertyDeclarationId, LonghandId, SourcePropertyDeclaration};
+use properties::{DeclarationSource, Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId};
+use properties::{PropertyDeclarationId, PropertyParserContext, LonghandId, SourcePropertyDeclaration};
 use properties::LonghandIdSet;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
 use servo_arc::Arc;
@@ -549,7 +549,11 @@ impl<'a, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for KeyframeListPars
         while let Some(declaration) = iter.next() {
             match declaration {
                 Ok(()) => {
-                    block.extend(iter.parser.declarations.drain(), Importance::Normal);
+                    block.extend(
+                        iter.parser.declarations.drain(),
+                        Importance::Normal,
+                        DeclarationSource::Parsing,
+                    );
                 }
                 Err((error, slice)) => {
                     iter.parser.declarations.clear();

ports/geckolib/glue.rs

@@ -113,7 +113,7 @@ use style::gecko_properties::style_structs;
 use style::invalidation::element::restyle_hints;
 use style::media_queries::{Device, MediaList, parse_media_query_list};
 use style::parser::{ParserContext, self};
-use style::properties::{CascadeFlags, ComputedValues, Importance};
+use style::properties::{CascadeFlags, ComputedValues, DeclarationSource, Importance};
 use style::properties::{IS_FIELDSET_CONTENT, IS_LINK, IS_VISITED_LINK, LonghandIdSet};
 use style::properties::{LonghandId, PropertyDeclaration, PropertyDeclarationBlock, PropertyId};
 use style::properties::{PropertyDeclarationId, ShorthandId};
@@ -2191,7 +2191,11 @@ pub extern "C" fn Servo_ParseProperty(
         Ok(()) => {
             let global_style_data = &*GLOBAL_STYLE_DATA;
             let mut block = PropertyDeclarationBlock::new();
-            block.extend(declarations.drain(), Importance::Normal);
+            block.extend(
+                declarations.drain(),
+                Importance::Normal,
+                DeclarationSource::CssOm,
+            );
             Arc::new(global_style_data.shared_lock.wrap(block)).into_strong()
         }
         Err(_) => RawServoDeclarationBlockStrong::null()
@@ -2483,7 +2487,11 @@ fn set_property(
         Ok(()) => {
             let importance = if is_important { Importance::Important } else { Importance::Normal };
             write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-                decls.extend_reset(source_declarations.drain(), importance)
+                decls.extend(
+                    source_declarations.drain(),
+                    importance,
+                    DeclarationSource::CssOm
+                )
             })
         },
         Err(_) => false,
@@ -2711,7 +2719,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetIdentStringValue(
         XLang => Lang(Atom::from(value)),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2757,7 +2765,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetKeywordValue(
         BorderLeftStyle => BorderStyle::from_gecko_keyword(value),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2778,7 +2786,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetIntValue(
         MozScriptLevel => MozScriptLevel::Relative(value),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2835,7 +2843,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPixelValue(
         },
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2875,7 +2883,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetLengthValue(
         MozScriptMinSize => MozScriptMinSize(nocalc),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2896,7 +2904,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetNumberValue(
         MozScriptLevel => MozScriptLevel::Absolute(value as i32),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2926,7 +2934,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPercentValue(
         FontSize => LengthOrPercentage::from(pc).into(),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2952,7 +2960,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetAutoValue(
         MarginLeft => auto,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -2974,7 +2982,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetCurrentColor(
         BorderLeftColor => cc,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -3002,7 +3010,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetColorValue(
         BackgroundColor => color,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal);
+        decls.push(prop, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -3023,7 +3031,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetFontFamily(
         if parser.is_exhausted() {
             let decl = PropertyDeclaration::FontFamily(family);
             write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-                decls.push(decl, Importance::Normal);
+                decls.push(decl, Importance::Normal, DeclarationSource::CssOm);
             })
         }
     }
@@ -3052,7 +3060,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetBackgroundImage(
             vec![Either::Second(Image::Url(url))]
         ));
         write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-            decls.push(decl, Importance::Normal);
+            decls.push(decl, Importance::Normal, DeclarationSource::CssOm);
         })
     }
 }
@@ -3068,7 +3076,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetTextDecorationColorOverride(
     decoration |= text_decoration_line::COLOR_OVERRIDE;
     let decl = PropertyDeclaration::TextDecorationLine(decoration);
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(decl, Importance::Normal);
+        decls.push(decl, Importance::Normal, DeclarationSource::CssOm);
     })
 }
 
@@ -3764,7 +3772,11 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(
                             id
                         }
                         PropertyDeclarationId::Custom(..) => {
-                            custom_properties.push(declaration.clone(), Importance::Normal);
+                            custom_properties.push(
+                                declaration.clone(),
+                                Importance::Normal,
+                                DeclarationSource::CssOm,
+                            );
                             continue;
                         }
                     };

tests/unit/style/keyframes.rs

@@ -5,6 +5,7 @@
 use cssparser::SourceLocation;
 use servo_arc::Arc;
 use style::properties::{LonghandId, LonghandIdSet, PropertyDeclaration, PropertyDeclarationBlock, Importance};
+use style::properties::DeclarationSource;
 use style::shared_lock::SharedRwLock;
 use style::stylesheets::keyframes_rule::{Keyframe, KeyframesAnimation, KeyframePercentage,  KeyframeSelector};
 use style::stylesheets::keyframes_rule::{KeyframesStep, KeyframesStepValue};
@@ -76,12 +77,14 @@ fn test_missing_property_in_initial_keyframe() {
             block.push(
                 PropertyDeclaration::Width(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block.push(
                 PropertyDeclaration::Height(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block
         }));
@@ -132,12 +135,14 @@ fn test_missing_property_in_final_keyframe() {
             block.push(
                 PropertyDeclaration::Width(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block.push(
                 PropertyDeclaration::Height(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block
         }));
@@ -195,12 +200,14 @@ fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() {
             block.push(
                 PropertyDeclaration::Width(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block.push(
                 PropertyDeclaration::Height(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
-                Importance::Normal
+                Importance::Normal,
+                DeclarationSource::Parsing,
             );
             block
         }));

tests/unit/style/stylesheets.rs

@@ -20,8 +20,8 @@ use style::error_reporting::{ParseErrorReporter, ContextualParseError};
 use style::media_queries::MediaList;
 use style::properties::Importance;
 use style::properties::{CSSWideKeyword, DeclaredValueOwned, PropertyDeclaration, PropertyDeclarationBlock};
-use style::properties::longhands;
-use style::properties::longhands::animation_timing_function;
+use style::properties::DeclarationSource;
+use style::properties::longhands::{self, animation_timing_function};
 use style::shared_lock::SharedRwLock;
 use style::stylesheets::{Origin, Namespaces};
 use style::stylesheets::{Stylesheet, StylesheetContents, NamespaceRule, CssRule, CssRules, StyleRule, KeyframesRule};
@@ -35,7 +35,7 @@ pub fn block_from<I>(iterable: I) -> PropertyDeclarationBlock
 where I: IntoIterator<Item=(PropertyDeclaration, Importance)> {
     let mut block = PropertyDeclarationBlock::new();
     for (d, i) in iterable {
-        block.push(d, i)
+        block.push(d, i, DeclarationSource::CssOm);
     }
     block
 }