From 605974a1122d34903e56358127c2d1d71509c143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 19 May 2017 15:40:44 +0200 Subject: [PATCH] Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r=heycam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: BHSxMJd0G0O Signed-off-by: Emilio Cobos Álvarez --- components/style/matching.rs | 76 +++++++++++++------ components/style/properties/gecko.mako.rs | 11 +++ .../style/properties/properties.mako.rs | 13 ++++ 3 files changed, 77 insertions(+), 23 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 0c4e53cfc4c..9b1711f3d48 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -614,9 +614,21 @@ trait PrivateMatchMethods: TElement { 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, @@ -802,12 +814,12 @@ trait PrivateMatchMethods: TElement { 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, @@ -1446,23 +1458,41 @@ pub trait MatchMethods : TElement { 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. diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 8ac9bfd3861..0736d34e97e 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -141,6 +141,13 @@ impl ComputedValues { 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 { @@ -4204,6 +4211,10 @@ clip-path <%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; diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 47022d30a6a..0f4ee1b6843 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1813,6 +1813,19 @@ impl ComputedValues { /// 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 {