Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip generating unnecessary @supports #878

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
555 changes: 550 additions & 5 deletions src/lib.rs

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/rules/font_palette_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ impl<'i> FontPaletteValuesRule<'i> {
// Generate color fallbacks.
let mut fallbacks = ColorFallbackKind::empty();
for o in override_colors {
fallbacks |= o.color.get_necessary_fallbacks(*context.targets);
fallbacks |= o.color.get_necessary_fallbacks(context.targets.current);
}

if fallbacks.contains(ColorFallbackKind::RGB) {
Expand Down
2 changes: 1 addition & 1 deletion src/rules/media.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl<'i, T: Clone> MediaRule<'i, T> {
self.query.transform_custom_media(self.loc, custom_media)?;
}

self.query.transform_resolution(*context.targets);
self.query.transform_resolution(context.targets.current);
Ok(self.rules.0.is_empty() || self.query.never_matches())
}
}
Expand Down
29 changes: 15 additions & 14 deletions src/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ use crate::printer::Printer;
use crate::rules::keyframes::KeyframesName;
use crate::selector::{is_compatible, is_equivalent, Component, Selector, SelectorList};
use crate::stylesheet::ParserOptions;
use crate::targets::Targets;
use crate::targets::TargetsWithSupportsScope;
use crate::traits::{AtRuleParser, ToCss};
use crate::values::string::CowArcStr;
use crate::vendor_prefix::VendorPrefix;
Expand Down Expand Up @@ -499,7 +499,7 @@ impl<'i, T: Visit<'i, T, V>, V: ?Sized + Visitor<'i, T>> Visit<'i, T, V> for Css
}

pub(crate) struct MinifyContext<'a, 'i> {
pub targets: &'a Targets,
pub targets: TargetsWithSupportsScope,
pub handler: &'a mut DeclarationHandler<'i>,
pub important_handler: &'a mut DeclarationHandler<'i>,
pub handler_context: PropertyHandlerContext<'i, 'a>,
Expand Down Expand Up @@ -535,7 +535,8 @@ impl<'i, T: Clone> CssRuleList<'i, T> {

macro_rules! set_prefix {
($keyframes: ident) => {
$keyframes.vendor_prefix = context.targets.prefixes($keyframes.vendor_prefix, Feature::AtKeyframes);
$keyframes.vendor_prefix =
context.targets.current.prefixes($keyframes.vendor_prefix, Feature::AtKeyframes);
};
}

Expand All @@ -559,7 +560,7 @@ impl<'i, T: Clone> CssRuleList<'i, T> {
set_prefix!(keyframes);
keyframe_rules.insert(keyframes.name.clone(), rules.len());

let fallbacks = keyframes.get_fallbacks(context.targets);
let fallbacks = keyframes.get_fallbacks(&context.targets.current);
rules.push(rule);
rules.extend(fallbacks);
continue;
Expand Down Expand Up @@ -647,14 +648,14 @@ impl<'i, T: Clone> CssRuleList<'i, T> {
// If some of the selectors in this rule are not compatible with the targets,
// we need to either wrap in :is() or split them into multiple rules.
let incompatible = if style.selectors.0.len() > 1
&& context.targets.should_compile_selectors()
&& !style.is_compatible(*context.targets)
&& context.targets.current.should_compile_selectors()
&& !style.is_compatible(context.targets.current)
{
// The :is() selector accepts a forgiving selector list, so use that if possible.
// Note that :is() does not allow pseudo elements, so we need to check for that.
// In addition, :is() takes the highest specificity of its arguments, so if the selectors
// have different weights, we need to split them into separate rules as well.
if context.targets.is_compatible(crate::compat::Feature::IsSelector)
if context.targets.current.is_compatible(crate::compat::Feature::IsSelector)
&& !style.selectors.0.iter().any(|selector| selector.has_pseudo_element())
&& style.selectors.0.iter().map(|selector| selector.specificity()).all_equal()
{
Expand All @@ -673,7 +674,7 @@ impl<'i, T: Clone> CssRuleList<'i, T> {
.cloned()
.partition::<SmallVec<[Selector; 1]>, _>(|selector| {
let list = SelectorList::new(smallvec![selector.clone()]);
is_compatible(&list.0, *context.targets)
is_compatible(&list.0, context.targets.current)
});
style.selectors = SelectorList::new(compatible);
incompatible
Expand Down Expand Up @@ -827,7 +828,7 @@ impl<'i, T: Clone> CssRuleList<'i, T> {

f.minify(context, parent_is_unused);

let fallbacks = f.get_fallbacks(*context.targets);
let fallbacks = f.get_fallbacks(context.targets.current);
rules.push(rule);
rules.extend(fallbacks);
continue;
Expand Down Expand Up @@ -931,8 +932,8 @@ fn merge_style_rules<'i, T>(
) -> bool {
// Merge declarations if the selectors are equivalent, and both are compatible with all targets.
if style.selectors == last_style_rule.selectors
&& style.is_compatible(*context.targets)
&& last_style_rule.is_compatible(*context.targets)
&& style.is_compatible(context.targets.current)
&& last_style_rule.is_compatible(context.targets.current)
&& style.rules.0.is_empty()
&& last_style_rule.rules.0.is_empty()
&& (!context.css_modules || style.loc.source_index == last_style_rule.loc.source_index)
Expand Down Expand Up @@ -961,7 +962,7 @@ fn merge_style_rules<'i, T>(
{
// If the new rule is unprefixed, replace the prefixes of the last rule.
// Otherwise, add the new prefix.
if style.vendor_prefix.contains(VendorPrefix::None) && context.targets.should_compile_selectors() {
if style.vendor_prefix.contains(VendorPrefix::None) && context.targets.current.should_compile_selectors() {
last_style_rule.vendor_prefix = style.vendor_prefix;
} else {
last_style_rule.vendor_prefix |= style.vendor_prefix;
Expand All @@ -970,9 +971,9 @@ fn merge_style_rules<'i, T>(
}

// Append the selectors to the last rule if the declarations are the same, and all selectors are compatible.
if style.is_compatible(*context.targets) && last_style_rule.is_compatible(*context.targets) {
if style.is_compatible(context.targets.current) && last_style_rule.is_compatible(context.targets.current) {
last_style_rule.selectors.0.extend(style.selectors.0.drain(..));
if style.vendor_prefix.contains(VendorPrefix::None) && context.targets.should_compile_selectors() {
if style.vendor_prefix.contains(VendorPrefix::None) && context.targets.current.should_compile_selectors() {
last_style_rule.vendor_prefix = style.vendor_prefix;
} else {
last_style_rule.vendor_prefix |= style.vendor_prefix;
Expand Down
4 changes: 2 additions & 2 deletions src/rules/style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ impl<'i, T> StyleRule<'i, T> {

pub(crate) fn update_prefix(&mut self, context: &mut MinifyContext<'_, 'i>) {
self.vendor_prefix = get_prefix(&self.selectors);
if self.vendor_prefix.contains(VendorPrefix::None) && context.targets.should_compile_selectors() {
self.vendor_prefix = downlevel_selectors(self.selectors.0.as_mut_slice(), *context.targets);
if self.vendor_prefix.contains(VendorPrefix::None) && context.targets.current.should_compile_selectors() {
self.vendor_prefix = downlevel_selectors(self.selectors.0.as_mut_slice(), context.targets.current);
}
}
}
Expand Down
43 changes: 40 additions & 3 deletions src/rules/supports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use crate::error::{MinifyError, ParserError, PrinterError};
use crate::parser::DefaultAtRule;
use crate::printer::Printer;
use crate::properties::PropertyId;
use crate::targets::Targets;
use crate::targets::{Features, FeaturesIterator, Targets};
use crate::traits::{Parse, ToCss};
use crate::values::color::ColorFallbackKind;
use crate::values::string::CowArcStr;
use crate::vendor_prefix::VendorPrefix;
#[cfg(feature = "visitor")]
Expand Down Expand Up @@ -42,8 +43,19 @@ impl<'i, T: Clone> SupportsRule<'i, T> {
context: &mut MinifyContext<'_, 'i>,
parent_is_unused: bool,
) -> Result<(), MinifyError> {
self.condition.set_prefixes_for_targets(&context.targets);
self.rules.minify(context, parent_is_unused)
let inserted = context.targets.enter_supports(self.condition.get_supported_features());
if inserted {
context.handler_context.targets = context.targets.current;
}

self.condition.set_prefixes_for_targets(&context.targets.current);
let result = self.rules.minify(context, parent_is_unused);

if inserted {
context.targets.exit_supports();
context.handler_context.targets = context.targets.current;
}
result
}
}

Expand Down Expand Up @@ -149,6 +161,31 @@ impl<'i> SupportsCondition<'i> {
_ => {}
}
}

fn get_supported_features(&self) -> Features {
const COLOR_P3_SUPPORTS_CONDITION: &str = ColorFallbackKind::P3.supports_condition_value();
const COLOR_LAB_SUPPORTS_CONDITION: &str = ColorFallbackKind::LAB.supports_condition_value();
fn get_supported_features_internal(value: &SupportsCondition) -> Option<Features> {
match value {
SupportsCondition::And(list) => list.iter().map(|c| get_supported_features_internal(c)).try_union_all(),
SupportsCondition::Declaration { property_id, value } => match property_id {
PropertyId::Color => Some(match value.as_ref() {
COLOR_P3_SUPPORTS_CONDITION => Features::P3Colors | Features::ColorFunction,
COLOR_LAB_SUPPORTS_CONDITION => Features::LabColors,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only work if the value is an exact match. Perhaps a future enhancement would be to attempt to parse the value, and extract the features from that. For example, if any lab() color was used, set that feature rather than only lab(0% 0 0)

Copy link
Contributor Author

@sapphi-red sapphi-red Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be nice to expand this detection in future.

_ => Features::empty(),
}),
_ => Some(Features::empty()),
},
// bail out if "not" or "or" exists for now
SupportsCondition::Not(_) | SupportsCondition::Or(_) => None,
SupportsCondition::Selector(_) | SupportsCondition::Unknown(_) => {
Some(Features::empty())
}
}
}

get_supported_features_internal(self).unwrap_or(Features::empty())
}
}

impl<'i> Parse<'i> for SupportsCondition<'i> {
Expand Down
4 changes: 2 additions & 2 deletions src/stylesheet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::error::{Error, ErrorLocation, MinifyErrorKind, ParserError, PrinterEr
use crate::parser::{DefaultAtRule, DefaultAtRuleParser, TopLevelRuleParser};
use crate::printer::Printer;
use crate::rules::{CssRule, CssRuleList, MinifyContext};
use crate::targets::{should_compile, Targets};
use crate::targets::{should_compile, Targets, TargetsWithSupportsScope};
use crate::traits::{AtRuleParser, ToCss};
use crate::values::string::CowArcStr;
#[cfg(feature = "visitor")]
Expand Down Expand Up @@ -242,7 +242,7 @@ where
};

let mut ctx = MinifyContext {
targets: &options.targets,
targets: TargetsWithSupportsScope::new(options.targets),
handler: &mut handler,
important_handler: &mut important_handler,
handler_context: context,
Expand Down
77 changes: 76 additions & 1 deletion src/targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#![allow(missing_docs)]

use std::borrow::Borrow;

use crate::vendor_prefix::VendorPrefix;
use bitflags::bitflags;
#[cfg(any(feature = "serde", feature = "nodejs"))]
Expand Down Expand Up @@ -136,7 +138,7 @@ fn parse_version(version: &str) -> Option<u32> {

bitflags! {
/// Features to explicitly enable or disable.
#[derive(Debug, Default, Clone, Copy)]
#[derive(Debug, Default, Clone, Copy, Hash, Eq, PartialEq)]
pub struct Features: u32 {
const Nesting = 1 << 0;
const NotSelectorList = 1 << 1;
Expand Down Expand Up @@ -165,6 +167,18 @@ bitflags! {
}
}

pub(crate) trait FeaturesIterator: Sized + Iterator {
fn try_union_all<T>(&mut self) -> Option<Features>
where
Self: Iterator<Item = Option<T>>,
T: Borrow<Features>,
{
self.try_fold(Features::empty(), |a, b| b.map(|b| a | *b.borrow()))
}
}

impl<I> FeaturesIterator for I where I: Iterator {}

/// Target browsers and features to compile.
#[derive(Debug, Clone, Copy, Default)]
pub struct Targets {
Expand Down Expand Up @@ -225,6 +239,67 @@ impl Targets {
}
}

#[derive(Debug)]
pub(crate) struct TargetsWithSupportsScope {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be part of Targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean putting the fields in Targets and making it stateful?

stack: Vec<Features>,
pub(crate) current: Targets,
}

impl TargetsWithSupportsScope {
pub fn new(targets: Targets) -> Self {
Self {
stack: Vec::new(),
current: targets,
}
}

/// Returns true if inserted
pub fn enter_supports(&mut self, features: Features) -> bool {
if features.is_empty() || self.current.exclude.contains(features) {
// Already excluding all features
return false;
}

let newly_excluded = features - self.current.exclude;
self.stack.push(newly_excluded);
self.current.exclude.insert(newly_excluded);
true
}

/// Should be only called if inserted
pub fn exit_supports(&mut self) {
if let Some(last) = self.stack.pop() {
self.current.exclude.remove(last);
}
}
}

#[test]
fn supports_scope_correctly() {
let mut targets = TargetsWithSupportsScope::new(Targets::default());
assert!(!targets.current.exclude.contains(Features::OklabColors));
assert!(!targets.current.exclude.contains(Features::LabColors));
assert!(!targets.current.exclude.contains(Features::P3Colors));

targets.enter_supports(Features::OklabColors | Features::LabColors);
assert!(targets.current.exclude.contains(Features::OklabColors));
assert!(targets.current.exclude.contains(Features::LabColors));

targets.enter_supports(Features::P3Colors | Features::LabColors);
assert!(targets.current.exclude.contains(Features::OklabColors));
assert!(targets.current.exclude.contains(Features::LabColors));
assert!(targets.current.exclude.contains(Features::P3Colors));

targets.exit_supports();
assert!(targets.current.exclude.contains(Features::OklabColors));
assert!(targets.current.exclude.contains(Features::LabColors));
assert!(!targets.current.exclude.contains(Features::P3Colors));

targets.exit_supports();
assert!(!targets.current.exclude.contains(Features::OklabColors));
assert!(!targets.current.exclude.contains(Features::LabColors));
}

macro_rules! should_compile {
($targets: expr, $feature: ident) => {
$targets.should_compile(
Expand Down
11 changes: 7 additions & 4 deletions src/values/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ pub enum FloatColor {

bitflags! {
/// A color type that is used as a fallback when compiling colors for older browsers.
#[derive(PartialEq, Eq, Clone, Copy)]
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub struct ColorFallbackKind: u8 {
/// An RGB color fallback.
const RGB = 0b01;
Expand Down Expand Up @@ -303,13 +303,16 @@ impl ColorFallbackKind {
*self | ColorFallbackKind::from_bits_truncate(self.bits() - 1)
}

pub(crate) fn supports_condition<'i>(&self) -> SupportsCondition<'i> {
let s = match *self {
pub(crate) const fn supports_condition_value(&self) -> &'static str {
match *self {
ColorFallbackKind::P3 => "color(display-p3 0 0 0)",
ColorFallbackKind::LAB => "lab(0% 0 0)",
_ => unreachable!(),
};
}
}

pub(crate) fn supports_condition<'i>(&self) -> SupportsCondition<'i> {
let s = self.supports_condition_value();
SupportsCondition::Declaration {
property_id: PropertyId::Color,
value: s.into(),
Expand Down
Loading