Page MenuHomePhabricator

Bug 1986629 - Lookup element attribute custom properties for css attr(). r=emilio
ClosedPublic

Authored by diegociudadreale on Dec 1 2025, 5:10 PM.
Referenced Files
Unknown Object (File)
Tue, Apr 14, 2:47 PM
Unknown Object (File)
Tue, Apr 14, 9:25 AM
Unknown Object (File)
Fri, Apr 10, 8:52 AM
Unknown Object (File)
Mon, Apr 6, 2:25 PM
Unknown Object (File)
Mon, Apr 6, 8:17 AM
Unknown Object (File)
Mon, Apr 6, 8:17 AM
Unknown Object (File)
Sun, Apr 5, 3:19 PM
Unknown Object (File)
Sun, Apr 5, 1:48 PM

Details

Event Timeline

phab-bot published this revision for review.Dec 1 2025, 5:12 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
emilio requested changes to this revision.Dec 1 2025, 5:58 PM
emilio added inline comments.
servo/components/style/custom_properties.rs
1032

nit: This style is fine, but you can use Option<impl TElement> if you fancy nowadays (and remove the generics)

2118–2119

Fairly sure you need this chunk as well right? Maybe factor it out like:

let substitution: Option<_> = match reference.substitution_kind {
    Var => {
       let registration = stylist.get_custom_property_registration(&reference.name);
       custom_properties.get(registration, &reference.name).map(|v| Substitution::from_value(v.to_variable_value()))
    },
    Env => {
        let device = stylist.device();
        device.environment().get(&reference.name, device, url_data).map(Substitution::from_value)
    },
    Attr => {
        ...
    }
};

if let Some(s) = substitution {
                // Skip references that are inside the outer variable (in fallback for example).
                while references
                    .next_if(|next_ref| next_ref.end <= reference.end)
                    .is_some()
                {}
    return Ok(s);
}
2126

using local_name you avoid the to_string here, and this can be:

if let Some(attr) = element.and_then(|e| e.get_attr(&reference.name)) {
    ...
}
2127

This does the right thing but in a very expensive way.

Instead, change Substitution::new to take a Cow<'a, str> and pass a Cow::Owned(attr) here.

This revision now requires changes to proceed.Dec 1 2025, 5:58 PM
diegociudadreale updated this revision to Diff 1164955.
diegociudadreale marked 4 inline comments as done.
emilio added a subscriber: Oriol.

What's the testing story here? I guess you're just stashing the string directly in there which probably doesn't do what you want. But seems ok for now, as we need to implement the whole type() stuff.

servo/components/style/properties/declaration_block.rs
365

Yeah, exactly the code-path I was concerned about hah

1017

This is not Gecko-specific so might need some changes for servo, cc @Oriol

Maybe we should have some sort of AttrProvider trait / struct or something that returned None for all attributes, and another that just wrapped a TElement.

This revision is now accepted and ready to land.Dec 1 2025, 8:47 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

servo/components/style/properties/declaration_block.rs
1017

Yes, please avoid using GeckoElement in non-Gecko-specific code. This AttrProvider idea seems reasonable.

diegociudadreale marked 3 inline comments as done.