Yes 2/3 failing tests occur because they expect substitution of the <attr-value> even if there's a cycle in the fallback (currently we catch the cycle in fallback which makes our attr invalid)

This is interesting, because Firefox and Safari are stricter in this regard compared to Chrome, even without attr. e.g.:

#foo {
  --z: 10px;
  --x: var(--y);
  --y: var(--z, var(--x));
  width: var(--y);
}

width will be 10px for Chrome, but not for Safari and Firefox. Glancing at the spec, I don't think it defines one way or the other?

EDIT: Ah, this spec discussion seems relevant: https://github.com/w3c/csswg-drafts/issues/11500
Seems like we resolved to not evaluate cycles in fallbacks if it's not taken. Probably requires references to be split into main references and fallback references for this to work...

(Here's the mapping) to the failing tests)

I think the other failing test is related to traversing too deep (i.e. if they don’t use type() syntax we can stop traversing its references), which causes an incorrect substitution.

(FWIW, same should apply for raw-string - we should just be not populating references for these, I'd think)

s/variable/attr/g

So that concern I don't think should be an issue as long as we handle the"shortcut" cases like we do for variables here and here.

Namely - trivial attr() should be already populated, and attr() referencing others should get substituted when it needs to be, because references get processed First-In-Last-Out in the stack here.

comment for linking purposes

@emilio, after discussing with @sukil -

<style>
#foo {
  width: attr(bar type(*));
}
</style>
<div id=foo bar="attr(baz type(*))" baz="attr(bar type(*))"></div>

We have only one non-custom property that does participate in a cycle.
I would think we want something like builder.cascade_attr() that does weave into the current substitution/cycle detection infra for custom properties, though.
... But agree that we should have that hide behind might_have_non_custom_dependency (Renamed to might_have_non_custom_or_attr_dependency and extend the check to WithVariables + any_attr.

Not a huge fan of what seems to be an implementation detail of CustomPropertiesMap leaking out. i.e. IndexMap being used is not known in this source file.
Perhaps better if we move this to custom_properties_map and name it just AllSubstitutionFunctions
We also should hide substitution_functions externally because if anyone decides to remove values, this will break, and have an insertion function that handles the index array insertion.

Isn't this just substitution_functions.size()?

Probably is_attr || registration. /*...*/ is fine

Out of curiosity (Since cycle detection isn't the main objective of this review, bug 1997338 is), what starts failing here?

I added a test to verify that same var and attr names have collision issues. Removing it, for now at least, want to think on that test some more.

Extra whitespace (Linting in general?)

Maybe

let local_name = if cfg!(feature = "gecko") {/*...*/} else {/*...*/};

Any reason for this change?

Yes so we were passing the following test for the incorrect reasons because width: 3 isn't valid right?

test_valid_attr('width', 'attr(data-foo type(*))', 'attr(data-bar type(<number>), 11)', '3');

So width: attr(data-foo type(*)) became approximately width: 1264px and width: 3 became width: 1264px --- which allowed us to pass the test without correctly substituting.

Probably should just DOM attributes.
Is the fact that it may be incomplete specific to attr()? Before CustomPropertiesBuilder::build() is called, var() refs may be incomplete as well, no?

Given how it's used, it should probably be Result<(), ()>

var() ref uses seen to skip lookup and insertion if the property was already referenced. Maybe worth something similar here.

I think this is almost there, maybe should mention why we need specific handling for attr().
Something like: "attr() in non-custom properties may then reference var(), which we need to track to support chained references and detect cycles"

can we move this down around line 1579 where custom properties get assigned as well?

(Aside, for bug 1997338)

It's likely that we will want to extend references_from_non_custom_properties to store names, as well as type (var or attr). Non-custom property references are only significant in the sense that it can loop back to properties in SingleNonCustomReference (Because attr() -> var() -> registered length property -> uses em/lh).

Should add comment to the effect of "Short circuit - attr() is always treated as if unregistered"

Er... Yeah, you're right. That is a strange test.
I think that's an uncontroversial change upstream, but let's add the details of this (How it used to set an invalid value) into the commit message.

I've rewritten it to now (hopefully) emphasis that even post CustomPropertiesBuilder::build(), this map could remain incomplete (i.e. attrs using attr-unit are excluded).

Had difficulties getting this to work with compiler type mismatch issues. Though mayhaps I was doing some silly stuff.

We do have an seen.attr get used in update_attributes_map() to skip seen attrs. In traverse we also check if context.map.get_attr(&next.name).is_none() {...} before calling map_attribute_name_to_value(). Lmk if that sounds sufficient enough.

Ah, uh, missed the comment in 1505. Any reason we shouldn't just substitute it right there? It just seems simpler, and this seems like a trip hazard from the caller perspective (It's supposed to contain _Computed_ substitution functions, after all).

Is there an issue making this OwnMap? Inherited stuff should not be an issue (Because you can't really declare inheriting attrs)

Right.. That's fun. You'd think it should still work given one branch gets compiled out.. but oh well

Ok, so maybe this function should either return the value Result<ComputedRegisteredValue, ()>, or should use like IndexMap::entry 1, and return if we did a lookup or not.

(Note substitution_functions simply being used to insert at the end, which generally tells me "it should just return the value and have the caller handle it")

Done :)

Right that makes sense, updated now to let caller handle it.

Flatten, perhaps?

I wonder if you can move maybe_note_attr_dependency here - basically:

if refs.any_attr {
  self.update_attributes_map(attribute_tracker, &v.value.variable_value);
  if !refs.any_var {
    return;
  }
}

I think that's logically equivalent, and may quell some of the concerning signs of regression I see on PerfCompare :(

Flatten unfortunately seems not implemented for Option<&Option<T>>, I alternatively used and_then().

I don't think this is going to be it, but maybe change this to:

if let PropertyDeclaration::WithVariables(v) = decl {
    return matches!(id, LonghandId::LineHeight | LonghandId::FontSize) || v.value.variable_value.references.any_attr;
}

It should shave a branch in the common case?

Why can't this be IndexMap<Key, ComputedRegisteredValue, FxBuildHasher>? You only insert Some(..) right?

Right! We disallow removing elements so we can just directly store ComputedRegisteredValue.

The extra clone() here is a bit unfortunate btw, would be nice to avoid...

I was able to address this (TIL & yay for Equivalent trait!), but to no effect :(

It ends up looking like:

#[derive(Clone, Debug, Hash, Eq, PartialEq)]
struct KeyRef<'a>(&'a Name, SubstitutionFunctionKind);

impl<'a> Equivalent<Key> for KeyRef<'a> {
    fn equivalent(&self, key: &Key) -> bool {
        *self.0 == key.0 && self.1 == key.1
    }
}
//...
self.0.get(&KeyRef(name, kind))

Going from inherited customproperties to all substitution functions seem pretty incorrect here.
(Doesn't hit this case, though)

Going from inherited customproperties to all substitution functions seem pretty incorrect here.
(Doesn't hit this case, though)

I guess it doesn't really change behavior because at the point we're handling container queries the builder really only has the inherited properties. But would be good to be explicit and use inherited_custom_properties() + None