Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 19 May 2017 15:40:44 +0200
changeset 581885 598bf0bc2749a11c0098d60cee2b0f87a53eaf67
parent 581228 d3ccffa738f78f18b504206ffae318a17f398579
child 581886 1a556bb5975676bd13ca2e01ef11b705a5ba1fc4
push id59904
push userbmo:emilio+bugs@crisal.io
push dateSat, 20 May 2017 01:35:22 +0000
reviewersheycam
bugs1366144
milestone55.0a1
Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r?heycam MozReview-Commit-ID: BHSxMJd0G0O
servo/components/style/matching.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/properties.mako.rs
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -574,19 +574,31 @@ trait PrivateMatchMethods: TElement {
         let (mut styles, restyle) = data.styles_and_restyle_mut();
         let mut pseudo_style = styles.pseudos.get_mut(pseudo).unwrap();
         let old_values = pseudo_style.values.take();
 
         let new_values =
             self.cascade_internal(context, &styles.primary, Some(pseudo_style));
 
         if let Some(old) = old_values {
-            // ::before and ::after are element-backed in Gecko, so they do
-            // the damage calculation for themselves.
-            if cfg!(feature = "servo") || !pseudo.is_before_or_after() {
+            // ::before and ::after are element-backed in Gecko, so they do the
+            // damage calculation for themselves, when there's an actual pseudo.
+            //
+            // NOTE(emilio): An alternative to this check is to just remove the
+            // pseudo-style entry here if new_values is display: none or has
+            // an ineffective content property...
+            //
+            // I think it's nice to handle it here instead for symmetry with how
+            // we handle display: none elements, but the other approach may be
+            // ok too?
+            let is_unexisting_before_or_after =
+                pseudo.is_before_or_after() &&
+                self.existing_style_for_restyle_damage(&old, Some(pseudo)).is_none();
+
+            if cfg!(feature = "servo") || is_unexisting_before_or_after {
                 self.accumulate_damage(&context.shared,
                                        restyle.unwrap(),
                                        &old,
                                        &new_values,
                                        Some(pseudo));
             }
         }
 
@@ -757,22 +769,22 @@ trait PrivateMatchMethods: TElement {
 
         // If an ancestor is already getting reconstructed by Gecko's top-down
         // frame constructor, no need to apply damage.
         if restyle.damage_handled.contains(RestyleDamage::reconstruct()) {
             restyle.damage = RestyleDamage::empty();
             return;
         }
 
-        // Add restyle damage, but only the bits that aren't redundant with respect
-        // to damage applied on our ancestors.
+        // Add restyle damage, but only the bits that aren't redundant with
+        // respect to damage applied on our ancestors.
         //
         // See https://bugzilla.mozilla.org/show_bug.cgi?id=1301258#c12
-        // for followup work to make the optimization here more optimal by considering
-        // each bit individually.
+        // for followup work to make the optimization here more optimal by
+        // considering each bit individually.
         if !restyle.damage.contains(RestyleDamage::reconstruct()) {
             let new_damage = self.compute_restyle_damage(&old_values,
                                                          &new_values,
                                                          pseudo);
             if !restyle.damage_handled.contains(new_damage) {
                 restyle.damage |= new_damage;
             }
         }
@@ -1380,33 +1392,51 @@ pub trait MatchMethods : TElement {
     /// pseudo-element, compute the restyle damage used to determine which
     /// kind of layout or painting operations we'll need.
     fn compute_restyle_damage(&self,
                               old_values: &ComputedValues,
                               new_values: &Arc<ComputedValues>,
                               pseudo: Option<&PseudoElement>)
                               -> RestyleDamage
     {
-        match self.existing_style_for_restyle_damage(old_values, pseudo) {
-            Some(ref source) => RestyleDamage::compute(source, new_values),
-            None => {
-                // If there's no style source, that likely means that Gecko
-                // couldn't find a style context. This happens with display:none
-                // elements, and probably a number of other edge cases that
-                // we don't handle well yet (like display:contents).
-                if new_values.get_box().clone_display() == display::T::none &&
-                    old_values.get_box().clone_display() == display::T::none {
-                    // The style remains display:none. No need for damage.
-                    RestyleDamage::empty()
-                } else {
-                    // Something else. Be conservative for now.
-                    RestyleDamage::reconstruct()
-                }
+        if let Some(source) = self.existing_style_for_restyle_damage(old_values, pseudo) {
+            return RestyleDamage::compute(source, new_values);
+        }
+
+        let new_style_is_display_none =
+            new_values.get_box().clone_display() == display::T::none;
+        let old_style_is_display_none =
+            old_values.get_box().clone_display() == display::T::none;
+
+        // If there's no style source, that likely means that Gecko couldn't
+        // find a style context.
+        //
+        // This happens with display:none elements, and not-yet-existing
+        // pseudo-elements.
+        if new_style_is_display_none && old_style_is_display_none {
+            // The style remains display:none. No need for damage.
+            return RestyleDamage::empty()
+        }
+
+        if pseudo.map_or(false, |p| p.is_before_or_after()) {
+            if (old_style_is_display_none ||
+                old_values.ineffective_content_property()) &&
+               (new_style_is_display_none ||
+                new_values.ineffective_content_property()) {
+                // The pseudo-element will remain undisplayed, so just avoid
+                // triggering any change.
+                return RestyleDamage::empty()
             }
+            return RestyleDamage::reconstruct()
         }
+
+        // Something else. Be conservative for now.
+        warn!("Reframing due to lack of old style source: {:?}, pseudo: {:?}",
+               self, pseudo);
+        RestyleDamage::reconstruct()
     }
 
     /// Cascade the eager pseudo-elements of this element.
     fn cascade_pseudos(&self,
                        context: &mut StyleContext<Self>,
                        mut data: &mut ElementData)
     {
         // Note that we've already set up the map of matching pseudo-elements
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -136,16 +136,23 @@ impl ComputedValues {
         })
     }
 
     #[inline]
     pub fn is_display_contents(&self) -> bool {
         self.get_box().clone_display() == longhands::display::computed_value::T::contents
     }
 
+    /// Returns true if the value of the `content` property would make a
+    /// pseudo-element not rendered.
+    #[inline]
+    pub fn ineffective_content_property(&self) -> bool {
+        self.get_counters().ineffective_content_property()
+    }
+
     % for style_struct in data.style_structs:
     #[inline]
     pub fn clone_${style_struct.name_lower}(&self) -> Arc<style_structs::${style_struct.name}> {
         self.${style_struct.ident}.clone()
     }
     #[inline]
     pub fn get_${style_struct.name_lower}(&self) -> &style_structs::${style_struct.name} {
         &self.${style_struct.ident}
@@ -4187,16 +4194,20 @@ clip-path
     }
 
     <% impl_app_units("column_rule_width", "mColumnRuleWidth", need_clone=True,
                       round_to_pixels=True) %>
 </%self:impl_trait>
 
 <%self:impl_trait style_struct_name="Counters"
                   skip_longhands="content counter-increment counter-reset">
+    pub fn ineffective_content_property(&self) -> bool {
+        self.gecko.mContents.is_empty()
+    }
+
     pub fn set_content(&mut self, v: longhands::content::computed_value::T) {
         use properties::longhands::content::computed_value::T;
         use properties::longhands::content::computed_value::ContentItem;
         use values::generics::CounterStyleOrNone;
         use gecko_bindings::structs::nsCSSValue;
         use gecko_bindings::structs::nsStyleContentType::*;
         use gecko_bindings::bindings::Gecko_ClearAndResizeStyleContents;
 
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1820,16 +1820,29 @@ impl ComputedValues {
     /// Servo for obvious reasons.
     pub fn has_moz_binding(&self) -> bool { false }
 
     /// Returns whether this style's display value is equal to contents.
     ///
     /// Since this isn't supported in Servo, this is always false for Servo.
     pub fn is_display_contents(&self) -> bool { false }
 
+    #[inline]
+    /// Returns whether the "content" property for the given style is completely
+    /// ineffective, and would yield an empty `::before` or `::after`
+    /// pseudo-element.
+    pub fn ineffective_content_property(&self) -> bool {
+        use properties::longhands::content::computed_value::T;
+        match self.get_counters().content {
+            T::normal |
+            T::none => true,
+            T::Content(ref items) => items.is_empty(),
+        }
+    }
+
     /// Whether the current style is multicolumn.
     #[inline]
     pub fn is_multicol(&self) -> bool {
         let style = self.get_column();
         match style.column_width {
             Either::First(_width) => true,
             Either::Second(_auto) => match style.column_count {
                 Either::First(_n) => true,