Cut property cascading if display:none is found

components/style/matching.rs

@@ -98,7 +98,7 @@ trait PrivateMatchMethods: TElement {
         primary_style: &Arc<ComputedValues>
     ) -> Option<Arc<ComputedValues>> {
         use context::CascadeInputs;
-        use style_resolver::{PseudoElementResolution, StyleResolverForElement};
+        use style_resolver::{PropertyCascading, PseudoElementResolution, StyleResolverForElement};
         use stylist::RuleInclusion;
 
         let rule_node = primary_style.rules();
@@ -120,8 +120,13 @@ trait PrivateMatchMethods: TElement {
 
         // Actually `PseudoElementResolution` doesn't really matter.
         let style =
-            StyleResolverForElement::new(*self, context, RuleInclusion::All, PseudoElementResolution::IfApplicable)
-                .cascade_style_and_visited_with_default_parents(inputs);
+            StyleResolverForElement::new(
+                *self,
+                context,
+                RuleInclusion::All,
+                PseudoElementResolution::IfApplicable,
+                PropertyCascading::ForceAll,
+            ).cascade_style_and_visited_with_default_parents(inputs);
 
         Some(style.0)
     }

components/style/properties/computed_value_flags.rs

@@ -62,6 +62,11 @@ bitflags! {
 
         /// A flag to mark a style which is a visited style.
         const IS_STYLE_IF_VISITED = 1 << 9;
+
+        /// A flag used to mark styles which don't have properties in other
+        /// category cascaded properly. Currently it is used for display:none
+        /// optimization.
+        const SKIPS_OTHER_CATEGORY = 1 << 10;
     }
 }
 

components/style/properties/properties.mako.rs

@@ -33,6 +33,7 @@ use logical_geometry::WritingMode;
 use media_queries::Device;
 use parser::ParserContext;
 #[cfg(feature = "gecko")] use properties::longhands::system_font::SystemFont;
+use properties::longhands::display::SpecifiedValue::None as display_none;
 use rule_cache::{RuleCache, RuleCacheConditions};
 use selector_parser::PseudoElement;
 use selectors::parser::SelectorParseErrorKind;
@@ -2130,6 +2131,12 @@ impl ComputedValues {
         self.flags.contains(ComputedValueFlags::IS_STYLE_IF_VISITED)
     }
 
+    /// Whether this style is a complete style, i.e. properties are not
+    /// skipped for optimization.
+    pub fn is_complete(&self) -> bool {
+        !self.flags.contains(ComputedValueFlags::SKIPS_OTHER_CATEGORY)
+    }
+
     /// Gets a reference to the rule node. Panic if no rule node exists.
     pub fn rules(&self) -> &StrongRuleNode {
         self.rules.as_ref().unwrap()
@@ -3086,6 +3093,11 @@ bitflags! {
         /// Whether we're computing the style of a link element that happens to
         /// be visited.
         const IS_VISITED_LINK = 1 << 5;
+
+        /// Whether skipping cascading properties in other category is allowed
+        /// when it is a display:none root. It should only be set if we are
+        /// doing traversal, and the result is not exposed to script.
+        const ALLOW_SKIPPING_OTHER_CATEGORY = 1 << 6;
     }
 }
 
@@ -3271,14 +3283,31 @@ where
     // To improve i-cache behavior, we outline the individual functions and use
     // virtual dispatch instead.
     let mut apply_reset = true;
+    let skip_other_category;
     % for category_to_cascade_now in ["early", "other"]:
         % if category_to_cascade_now == "early":
             // Pull these out so that we can compute them in a specific order
             // without introducing more iterations.
             let mut font_size = None;
             let mut font_family = None;
+            // Pull these out in early category so that we can cut cascading
+            // for display:none element.
+            let mut has_display_none = None;
+            % if product == "gecko":
+                let mut moz_binding = None;
+            % endif
         % endif
         for (declaration, cascade_level) in iter_declarations() {
+            % if category_to_cascade_now == "other":
+                // Skip other category if we don't need them. It is easier to
+                // write here rather than wrapping the whole loop.
+                // XXX Actually rustc cannot move this check out from loop
+                // body with opt-level=2, see rust-lang/rust#46542.
+                if skip_other_category {
+                    break;
+                }
+            % endif
+
             let declaration_id = declaration.id();
             let longhand_id = match declaration_id {
                 PropertyDeclarationId::Longhand(id) => id,
@@ -3297,6 +3326,32 @@ where
                 continue;
             }
 
+            % if category_to_cascade_now == "early":
+                // Pull out display:none as well as -moz-binding for optimization.
+                match longhand_id {
+                    LonghandId::Display => {
+                        if has_display_none.is_none() {
+                            has_display_none = Some(*declaration == PropertyDeclaration::Display(display_none));
+                        }
+                    }
+                    % if product == "gecko":
+                    LonghandId::MozBinding => {
+                        if moz_binding.is_none() {
+                            if let PropertyDeclaration::MozBinding(ref value) = *declaration {
+                                moz_binding = Some(value.clone());
+                            } else {
+                                // In case we have variable / CSS-wide keyword value for
+                                // -moz-binding, just disable display:none optimization.
+                                has_display_none = Some(false);
+                                moz_binding = Some(Default::default());
+                            }
+                        }
+                    }
+                    % endif
+                    _ => {}
+                }
+            % endif
+
             if
                 % if category_to_cascade_now == "early":
                     !
@@ -3486,9 +3541,38 @@ where
             % endif
             }
 
+            % if product == "gecko":
+            skip_other_category = has_display_none.unwrap_or(false) &&
+                flags.contains(CascadeFlags::ALLOW_SKIPPING_OTHER_CATEGORY) &&
+                // Don't apply this optimization to pseudo-elements for now.
+                pseudo.is_none();
+            % else:
+            // FIXME It is disabled for Servo for now until we can fix
+            // getComputedStyle for Servo as well.
+            skip_other_category = false;
+            % endif
+
             if let Some(style) = rule_cache.and_then(|c| c.find(&context.builder)) {
                 context.builder.copy_reset_from(style);
                 apply_reset = false;
+            } else if skip_other_category {
+                // display:none optimization would skip cascading the whole
+                // other category, and thus if we couldn't find reset styles
+                // from the rule cache, we need to cascade display here.
+                let display = PropertyDeclaration::Display(display_none);
+                CASCADE_PROPERTY[LonghandId::Display as usize](&display, &mut context);
+                % if product == "gecko":
+                if let Some(value) = moz_binding {
+                    let moz_binding = PropertyDeclaration::MozBinding(value);
+                    CASCADE_PROPERTY[LonghandId::MozBinding as usize](&moz_binding, &mut context);
+                }
+                % endif
+                context.builder.flags.insert(ComputedValueFlags::SKIPS_OTHER_CATEGORY);
+                // XXX Should we make it uncacheable? If stuff which requires
+                // complete style data, e.g. getComputedStyle, gets this cached
+                // style, it would return wrong value. But it seems that we
+                // are currently not sharing this cache with that usage so we
+                // should be fine.
             }
         % endif // category == "early"
     % endfor

components/style/style_resolver.rs

@@ -28,6 +28,18 @@ pub enum PseudoElementResolution {
     Force,
 }
 
+/// Whether we can skip cascading of some properties if not
+/// necessary, or we have to cascade everything.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum PropertyCascading {
+    /// Allow skipping properties in other category if the
+    /// element has display:none.
+    AllowSkippingForDisplayNone,
+    /// Force cascading all properties so that the result
+    /// can be exposed to outside observer, e.g. scripts.
+    ForceAll,
+}
+
 /// A struct that takes care of resolving the style of a given element.
 pub struct StyleResolverForElement<'a, 'ctx, 'le, E>
 where
@@ -39,6 +51,7 @@ where
     context: &'a mut StyleContext<'ctx, E>,
     rule_inclusion: RuleInclusion,
     pseudo_resolution: PseudoElementResolution,
+    prop_cascading: PropertyCascading,
     _marker: ::std::marker::PhantomData<&'le E>,
 }
 
@@ -138,12 +151,14 @@ where
         context: &'a mut StyleContext<'ctx, E>,
         rule_inclusion: RuleInclusion,
         pseudo_resolution: PseudoElementResolution,
+        prop_cascading: PropertyCascading,
     ) -> Self {
         Self {
             element,
             context,
             rule_inclusion,
             pseudo_resolution,
+            prop_cascading,
             _marker: ::std::marker::PhantomData,
         }
     }
@@ -573,6 +588,10 @@ where
             cascade_flags.insert(CascadeFlags::IS_ROOT_ELEMENT);
         }
 
+        if self.prop_cascading == PropertyCascading::AllowSkippingForDisplayNone {
+            cascade_flags.insert(CascadeFlags::ALLOW_SKIPPING_OTHER_CATEGORY);
+        }
+
         let implemented_pseudo = self.element.implemented_pseudo_element();
         let pseudo = pseudo.or(implemented_pseudo.as_ref());
 

components/style/traversal.rs

@@ -13,7 +13,7 @@ use selector_parser::PseudoElement;
 use selectors::NthIndexCache;
 use sharing::StyleSharingTarget;
 use smallvec::SmallVec;
-use style_resolver::{PseudoElementResolution, StyleResolverForElement};
+use style_resolver::{PropertyCascading, PseudoElementResolution, StyleResolverForElement};
 use stylist::RuleInclusion;
 use traversal_flags::TraversalFlags;
 
@@ -311,7 +311,9 @@ where
     debug_assert!(rule_inclusion == RuleInclusion::DefaultOnly ||
                   ignore_existing_style ||
                   pseudo.map_or(false, |p| p.is_before_or_after()) ||
-                  element.borrow_data().map_or(true, |d| !d.has_styles()),
+                  element.borrow_data().map_or(true, |d| {
+                      !d.has_styles() || !d.styles.primary().is_complete()
+                  }),
                   "Why are we here?");
     let mut ancestors_requiring_style_resolution = SmallVec::<[E; 16]>::new();
 
@@ -324,8 +326,10 @@ where
         if rule_inclusion == RuleInclusion::All && !ignore_existing_style {
             if let Some(data) = current.borrow_data() {
                 if let Some(ancestor_style) = data.styles.get_primary() {
-                    style = Some(ancestor_style.clone());
-                    break;
+                    if ancestor_style.is_complete() {
+                        style = Some(ancestor_style.clone());
+                        break;
+                    }
                 }
             }
         }
@@ -357,11 +361,16 @@ where
         // Actually `PseudoElementResolution` doesn't really matter here.
         // (but it does matter below!).
         let primary_style =
-            StyleResolverForElement::new(*ancestor, context, rule_inclusion, PseudoElementResolution::IfApplicable)
-                .resolve_primary_style(
-                    style.as_ref().map(|s| &**s),
-                    layout_parent_style.as_ref().map(|s| &**s)
-                );
+            StyleResolverForElement::new(
+                *ancestor,
+                context,
+                rule_inclusion,
+                PseudoElementResolution::IfApplicable,
+                PropertyCascading::ForceAll,
+            ).resolve_primary_style(
+                style.as_ref().map(|s| &**s),
+                layout_parent_style.as_ref().map(|s| &**s)
+            );
 
         let is_display_contents = primary_style.style().is_display_contents();
 
@@ -374,7 +383,13 @@ where
     }
 
     context.thread_local.bloom_filter.assert_complete(element);
-    StyleResolverForElement::new(element, context, rule_inclusion, PseudoElementResolution::Force)
+    StyleResolverForElement::new(
+        element,
+        context,
+        rule_inclusion,
+        PseudoElementResolution::Force,
+        PropertyCascading::ForceAll,
+    )
         .resolve_style(
             style.as_ref().map(|s| &**s),
             layout_parent_style.as_ref().map(|s| &**s)
@@ -607,7 +622,8 @@ where
                                 element,
                                 context,
                                 RuleInclusion::All,
-                                PseudoElementResolution::IfApplicable
+                                PseudoElementResolution::IfApplicable,
+                                PropertyCascading::AllowSkippingForDisplayNone,
                             );
 
                         resolver.resolve_style_with_default_parents()
@@ -636,7 +652,8 @@ where
                     element,
                     context,
                     RuleInclusion::All,
-                    PseudoElementResolution::IfApplicable
+                    PseudoElementResolution::IfApplicable,
+                    PropertyCascading::AllowSkippingForDisplayNone,
                 );
 
             resolver.cascade_styles_with_default_parents(cascade_inputs)
@@ -652,7 +669,8 @@ where
                         element,
                         context,
                         RuleInclusion::All,
-                        PseudoElementResolution::IfApplicable
+                        PseudoElementResolution::IfApplicable,
+                        PropertyCascading::AllowSkippingForDisplayNone,
                     );
 
                 resolver.cascade_styles_with_default_parents(cascade_inputs)

components/style/values/specified/mod.rs

@@ -512,9 +512,15 @@ impl Parse for PositiveInteger {
 /// PositiveInteger | auto
 pub type PositiveIntegerOrAuto = Either<PositiveInteger, Auto>;
 
-#[allow(missing_docs)]
+/// <url> | none
 pub type UrlOrNone = Either<SpecifiedUrl, None_>;
 
+impl Default for UrlOrNone {
+    fn default() -> Self {
+        Either::Second(None_)
+    }
+}
+
 /// The specified value of a grid `<track-breadth>`
 pub type TrackBreadth = GenericTrackBreadth<LengthOrPercentage>;
 

ports/geckolib/glue.rs

@@ -804,7 +804,8 @@ fn resolve_rules_for_element_with_context<'a>(
     mut context: StyleContext<GeckoElement<'a>>,
     rules: StrongRuleNode
 ) -> Arc<ComputedValues> {
-    use style::style_resolver::{PseudoElementResolution, StyleResolverForElement};
+    use style::style_resolver::{PropertyCascading, PseudoElementResolution};
+    use style::style_resolver::StyleResolverForElement;
 
     // This currently ignores visited styles, which seems acceptable, as
     // existing browsers don't appear to animate visited styles.
@@ -815,11 +816,13 @@ fn resolve_rules_for_element_with_context<'a>(
         };
 
     // Actually `PseudoElementResolution` doesn't matter.
-    StyleResolverForElement::new(element,
-                                 &mut context,
-                                 RuleInclusion::All,
-                                 PseudoElementResolution::IfApplicable)
-        .cascade_style_and_visited_with_default_parents(inputs).0
+    StyleResolverForElement::new(
+        element,
+        &mut context,
+        RuleInclusion::All,
+        PseudoElementResolution::IfApplicable,
+        PropertyCascading::ForceAll,
+    ).cascade_style_and_visited_with_default_parents(inputs).0
 }
 
 #[no_mangle]
@@ -3550,7 +3553,13 @@ pub extern "C" fn Servo_ResolveStyleLazily(
                     /* matching_func = */ None,
                 )
             }
-            None => Some(styles.primary().clone()),
+            None => {
+                if styles.primary().is_complete() {
+                    Some(styles.primary().clone())
+                } else {
+                    None
+                }
+            }
         }
     };