Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r?heycam
MozReview-Commit-ID: BHSxMJd0G0O
--- 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,