From 13d12d0d5f015a7dea164599cf061a2b9d332577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 26 Jan 2020 22:57:21 +0000 Subject: [PATCH] style: Tweak background-color and color handling in ignored-colors mode. We're resetting `color` to the default color when there's a declaration that applies in order to make stuff like this:
Red
To not show transparent. But the behavior we want is more like "override with default color iff there's no other declaration that would set the color from an user or UA sheet". This implements that behavior, plus avoids it if we're not inheriting from transparent, so that stuff like this preserves the behavior from before bug 844349: Should be the red color Differential Revision: https://phabricator.services.mozilla.com/D60391 --- components/style/properties/cascade.rs | 117 +++++++++++++++++-------- 1 file changed, 79 insertions(+), 38 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index f9f17a57121..461bc0c1fce 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -333,27 +333,37 @@ where context.builder.build() } -fn should_ignore_declaration_when_ignoring_document_colors( - device: &Device, +/// How should a declaration behave when ignoring document colors? +enum DeclarationApplication { + /// We should apply the declaration. + Apply, + /// We should ignore the declaration. + Ignore, + /// We should apply the following declaration, only if any other declaration + /// hasn't set it before. + ApplyUnlessOverriden(PropertyDeclaration), +} + +fn application_when_ignoring_colors( + builder: &StyleBuilder, longhand_id: LonghandId, origin: Origin, - pseudo: Option<&PseudoElement>, - declaration: &mut Cow, -) -> bool { + declaration: &PropertyDeclaration, +) -> DeclarationApplication { if !longhand_id.ignored_when_document_colors_disabled() { - return false; + return DeclarationApplication::Apply; } let is_ua_or_user_rule = matches!(origin, Origin::User | Origin::UserAgent); if is_ua_or_user_rule { - return false; + return DeclarationApplication::Apply; } // Don't override background-color on ::-moz-color-swatch. It is set as an // author style (via the style attribute), but it's pretty important for it // to show up for obvious reasons :) - if pseudo.map_or(false, |p| p.is_color_swatch()) && longhand_id == LonghandId::BackgroundColor { - return false; + if builder.pseudo.map_or(false, |p| p.is_color_swatch()) && longhand_id == LonghandId::BackgroundColor { + return DeclarationApplication::Apply; } // Treat background-color a bit differently. If the specified color is @@ -365,30 +375,41 @@ fn should_ignore_declaration_when_ignoring_document_colors( // a background image, if we're ignoring document colors). // Here we check backplate status to decide if ignoring background-image // is the right decision. - let (is_background, is_transparent) = match **declaration { - PropertyDeclaration::BackgroundColor(ref color) => (true, color.is_transparent()), - PropertyDeclaration::Color(ref color) => (false, color.0.is_transparent()), + match *declaration { + PropertyDeclaration::BackgroundColor(ref color) => { + if color.is_transparent() { + return DeclarationApplication::Apply; + } + let color = builder.device.default_background_color(); + DeclarationApplication::ApplyUnlessOverriden( + PropertyDeclaration::BackgroundColor(color.into()) + ) + } + PropertyDeclaration::Color(ref color) => { + if color.0.is_transparent() { + return DeclarationApplication::Apply; + } + if builder.get_parent_inherited_text().clone_color().alpha != 0 { + return DeclarationApplication::Ignore; + } + let color = builder.device.default_color(); + DeclarationApplication::ApplyUnlessOverriden( + PropertyDeclaration::Color(specified::ColorPropertyValue(color.into())) + ) + }, // In the future, if/when we remove the backplate pref, we can remove this // special case along with the 'ignored_when_colors_disabled=True' mako line // for the "background-image" property. #[cfg(feature = "gecko")] - PropertyDeclaration::BackgroundImage(..) => return !static_prefs::pref!("browser.display.permit_backplate"), - _ => return true, - }; - - if is_transparent { - return false; + PropertyDeclaration::BackgroundImage(..) => { + if static_prefs::pref!("browser.display.permit_backplate") { + DeclarationApplication::Apply + } else { + DeclarationApplication::Ignore + } + }, + _ => DeclarationApplication::Ignore, } - - if is_background { - let color = device.default_background_color(); - *declaration.to_mut() = PropertyDeclaration::BackgroundColor(color.into()); - } else { - let color = device.default_color(); - *declaration.to_mut() = PropertyDeclaration::Color(specified::ColorPropertyValue(color.into())); - } - - false } struct Cascade<'a, 'b: 'a> { @@ -465,6 +486,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { ); let ignore_colors = !self.context.builder.device.use_document_colors(); + let mut declarations_to_apply_unless_overriden = + SmallVec::<[PropertyDeclaration; 2]>::new(); for (declaration, origin) in declarations { let declaration_id = declaration.id(); @@ -506,21 +529,25 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { continue; } - let mut declaration = self.substitute_variables_if_needed(declaration); + let declaration = self.substitute_variables_if_needed(declaration); - // When document colors are disabled, skip properties that are - // marked as ignored in that mode, unless they come from a UA or - // user style sheet. + // When document colors are disabled, do special handling of + // properties that are marked as ignored in that mode. if ignore_colors { - let should_ignore = should_ignore_declaration_when_ignoring_document_colors( - self.context.builder.device, + let application = application_when_ignoring_colors( + &self.context.builder, longhand_id, origin, - self.context.builder.pseudo, - &mut declaration, + &declaration, ); - if should_ignore { - continue; + + match application { + DeclarationApplication::Ignore => continue, + DeclarationApplication::Apply => {}, + DeclarationApplication::ApplyUnlessOverriden(decl) => { + declarations_to_apply_unless_overriden.push(decl); + continue; + } } } @@ -558,6 +585,20 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { self.apply_declaration(longhand_id, &*declaration); } + if ignore_colors { + for declaration in declarations_to_apply_unless_overriden.iter() { + let longhand_id = match declaration.id() { + PropertyDeclarationId::Longhand(id) => id, + PropertyDeclarationId::Custom(..) => unreachable!(), + }; + debug_assert!(!longhand_id.is_logical()); + if self.seen.contains(longhand_id) { + continue; + } + self.apply_declaration(longhand_id, declaration); + } + } + if Phase::is_early() { self.fixup_font_stuff(); self.compute_writing_mode();