Skip to content

css: do not add transition entries the declaration already lists #38692

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.

Sign up for GitHub

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

css: do not add transition entries the declaration already lists #38692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 51 additions & 27 deletions src/css/properties/transition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use crate::VendorPrefix;
use crate::compat;
use crate::prefixes::Feature;
use crate::targets::Targets;

/// A value for the [transition](https://www.w3.org/TR/2018/WD-css-transitions-1-20181011/#transition-shorthand-property) property.
#[derive(Clone, PartialEq)]
Expand Down Expand Up @@ -510,6 +511,10 @@
}
}

// Entries the declaration already spells out (`-webkit-transform, transform`,
// which is also what this handler's own output looks like when a merged rule
// is minified again) must not be generated a second time below.
let as_written: SmallList<PropertyId, 4> = properties.iter().copied().collect();
let mut rtl_properties: Option<SmallList<PropertyId, 1>> = None;
let mut i: u32 = 0;

Expand Down Expand Up @@ -543,38 +548,23 @@
}
_ => {
let index = i;
// Expand vendor prefixes for targets.
properties.slice_mut()[index as usize]
.set_prefixes_for_targets(&context.targets);

// Expand mask properties, which use different vendor-prefixed names.
let targets = &context.targets;
expand_prefixes(properties.r#mut(index), &as_written, targets);
if let Some(property_id) =
masking::get_webkit_mask_property(properties.at(index))
webkit_mask_property_to_insert(properties.at(index), &as_written, targets)
{
if context
.targets
.prefixes(VendorPrefix::NONE, Feature::MaskBorder)
.contains(VendorPrefix::WEBKIT)
{
properties.insert(index, property_id);
i += 1;
}
properties.insert(index, property_id);
i += 1;

Check notice on line 557 in src/css/properties/transition.rs

View check run for this annotation

Claude / Claude Code Review

Pre-existing: mask-box-image insertion misaligns durations/delays/timing with subsequent entries 

Pre-existing (not introduced by this PR, and strictly mitigated by it): when the `-webkit-mask-box-image*` entry is inserted here, `durations`/`delays`/`timing_functions` in `flush` are not grown to match, so `get_transitions` cycles and shifts every subsequent entry — safari 15 + `transition: mask-border .5s, opacity .1s` comes out as `-webkit-mask-box-image .5s, mask-border .1s, opacity .5s`. The `LogicalPropertyId::Block`/`Inline` `replace()` arms (e.g. `inset-block` → two entries) have the s
Comment on lines +551 to +557

@claude claude Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟣 Pre-existing (not introduced by this PR, and strictly mitigated by it): when the -webkit-mask-box-image* entry is inserted here, durations/delays/timing_functions in flush are not grown to match, so get_transitions cycles and shifts every subsequent entry — safari 15 + transition: mask-border .5s, opacity .1s comes out as -webkit-mask-box-image .5s, mask-border .1s, opacity .5s. The LogicalPropertyId::Block/Inline replace() arms (e.g. inset-block → two entries) have the same shape. Noting only because the refactored block is where a fix would land and the new tests all use single-entry or equal-duration lists that hide the cycling; not blocking.

Extended reasoning...

What the bug is

expand_properties receives only the properties list (flush() calls it as expand_properties(&mut p.0, arena, context)) and grows it in place via properties.insert(index, property_id) when a mask-border* entry needs a -webkit-mask-box-image* counterpart. The durations, delays, and timing_functions lists that flush holds alongside it are never touched, so after the insert the property list is one entry longer than the value lists it is paired with.

get_transitions then walks properties.slice() and pairs each entry with durations.at(durations_idx), bumping the index once per property via cycle_bump(&mut durations_idx, durations.len()). When properties.len() > durations.len(), that cycling shifts every entry after the insertion point onto its neighbor's value and wraps the last entry back to the first slot.

Step-by-step repro

Input (targets: safari 15, which needs the -webkit-mask-box-image insert):

.foo { transition: mask-border .5s, opacity .1s }
  1. handle_property splits the shorthand into properties = [MaskBorder, Opacity], durations = [.5s, .1s] (delays/timing similarly length-2).
  2. flush calls expand_properties. webkit_mask_property_to_insert returns WebKitMaskBoxImage (as_written does not contain it), so properties.insert(0, WebKitMaskBoxImage) runs. Now properties = [WebKitMaskBoxImage, MaskBorder, Opacity] — length 3.
  3. durations is still [.5s, .1s] — length 2; flush never grew it.
  4. get_transitions iterates:
    • WebKitMaskBoxImage → durations[0] = .5s, bump → idx 1
    • MaskBorder → durations[1] = .1s, bump → idx 0 (cycle)
    • Opacity → durations[0] = .5s
  5. Output: transition: -webkit-mask-box-image .5s, mask-border .1s, opacity .5s — mask-border lost its .5s and opacity was rewritten from .1s to .5s.

The prefix-expansion path (expand_prefixes) is not affected: a multi-bit VendorPrefix stays a single properties[] entry and get_transitions fans it out with one durations_idx increment. Only the mask insert (and the LogicalPropertyId::Block/Inline arms whose replace() calls insert_slice for multi-entry expansions like InsetBlock → [Top, Bottom]) add real list entries.

Why this is pre-existing and why the new tests don't catch it

The removed code did the identical properties.insert(index, property_id) without growing the value lists; this PR only wraps the condition in webkit_mask_property_to_insert and adds the as_written dedup. So this is not a regression — and the PR strictly reduces how often it fires, since the second-pass insert (merged rules, :lang() rules) is now skipped when the entry is already listed. The upstream lightningcss code the file was ported from has the same behavior.

The 13 new prefix_tests all use either a single mask-border entry (durations.len() == 1, so cycling is a no-op) or equal durations across every entry (.2s, .2s), both of which are invariant under the shift. A two-entry list with distinct durations, e.g. transition: mask-border .5s, opacity .1s at safari 15, would exercise it.

What a fix would look like

The value lists need the entry duplicated at the same index the property was inserted at (so the inserted -webkit-mask-box-image reuses mask-border's duration and everything after it stays paired). That means either passing the value lists into expand_properties and inserting alongside the property, or having expand_properties return the insertion indices for flush to apply to all four lists. The same treatment would cover the multi-entry Block/Inline replace() expansions. Not asking for it here — it's a separate change from the fixed-point fix this PR makes, and it exists upstream too — but flagging it since the PR description's "no entry is removed or reordered, so each entry keeps its own duration" holds for expand_prefixes but not for this insert.

}

if let Some(rtl_props) = &mut rtl_properties {
rtl_props.slice_mut()[index as usize]
.set_prefixes_for_targets(&context.targets);

if let Some(property_id) =
masking::get_webkit_mask_property(rtl_props.at(index))
{
if context
.targets
.prefixes(VendorPrefix::NONE, Feature::MaskBorder)
.contains(VendorPrefix::WEBKIT)
{
rtl_props.insert(index, property_id);
}
expand_prefixes(rtl_props.r#mut(index), &as_written, targets);
if let Some(property_id) = webkit_mask_property_to_insert(
rtl_props.at(index),
&as_written,
targets,
) {
rtl_props.insert(index, property_id);
}
}
i += 1;
Expand All @@ -585,6 +575,40 @@
rtl_properties
}

/// Expands the vendor prefixes of `property_id` for the targets, minus the
/// prefixes that other entries of the same property in `as_written` already
/// carry: `-webkit-transform, transform` expands `transform` to the remaining
/// prefixes only, so each entry keeps its own duration, delay and timing function.
fn expand_prefixes(property_id: &mut PropertyId, as_written: &[PropertyId], targets: &Targets) {
let written = property_id.prefix();
property_id.set_prefixes_for_targets(targets);
let added = property_id.prefix().difference(written);
if added.is_empty() {
return;
}

let listed = as_written
.iter()
.filter(|p| p.tag() == property_id.tag())
.fold(VendorPrefix::empty(), |listed, p| listed | p.prefix());
*property_id = property_id.with_prefix(property_id.prefix().difference(added & listed));
}

/// The `-webkit-` mask property that animates alongside `property_id` for the
/// targets (`mask-border` -> `-webkit-mask-box-image`, which has a different
/// name rather than a prefix), unless the declaration already lists it.
fn webkit_mask_property_to_insert(
property_id: &PropertyId,
as_written: &[PropertyId],
targets: &Targets,
) -> Option<PropertyId> {
let webkit = masking::get_webkit_mask_property(property_id)?;
let needed = targets
.prefixes(VendorPrefix::NONE, Feature::MaskBorder)
.contains(VendorPrefix::WEBKIT);
(needed && !as_written.contains(&webkit)).then_some(webkit)
}

enum LogicalPropertyId {
None,
Block(compat::Feature, &'static [PropertyId]),
Expand Down
Loading