I realize this is still a draft, so perhaps you were going to change this, but this should use something like the following for the underlying type:

Variant<OverflowClipReferenceBox, Length, SpaceSeparatedTuple<OverflowClipReferenceBox, Length>>

That way, only the valid state is initialized.

Variant's a lot better than what I wrote. I was mostly following StyleAspectRatio.h and StyleAspectRatio.cpp for my implementation. I'll change it. It makes the rest of the Class a lot cleaner.

You shouldn't need this if you use a Variant, but even if you decide not to, this can be internal and private to OverflowClipMargin.

I'll use variant and remove this.

These aren't needed for types that implemented in CSSPrimitiveValueMappings.h‎ using the macros. (I haven't yet removed all the ones I could here, but this is a good reminder to me to do so).

Sure, I'll remove it.

I would consider calling this VisualBox, and adding a spec link to https://drafts.csswg.org/css-box-4/#typedef-visual-box.

Great suggestion! I'll add VisualBox and update it here.

This should be a dynamic_downcast. requiredDowncast will tell the builder the conversion failed if the cast isn't successful. That means the CSSValueList code below will never be successfully run.

Makes sense. I'll update it.

After this, you should add:

This will make sure that any other types of primitive values force an invalid style resolution.

Sure, I'll update it. I want to confirm my understanding of what this actually does. Essentially the setCurrentPropertyInvalidAtComputedValueTime will make sure that the overflow-clip-margin's computed value is set to 0px if it's invalid.

Take for example StyleBorderRadius.cpp. Why is setCurrentPropertyInvalidAtComputedValueTime not called here - https://searchfox.org/wubkat/source/Source/WebCore/style/values/borders/StyleBorderRadius.cpp#71 ?

And for StyleWebKitTextStrokeWidth.cpp, it's only called once in line 58 but not in line 42 -
https://searchfox.org/wubkat/source/Source/WebCore/style/values/non-standard/StyleWebKitTextStrokeWidth.cpp#38. Why is that the case?

The setCurrentPropertyInvalidAtComputedValueTime() stuff is largely "belt and suspenders" precautions against a specific weird edge case where rather than the CSSValue coming from the CSSParser, it comes from the CSS typedom. This can happen by using things like the StylePropertyMap interface to set CSS values. There are plans to fix this in a more comprehensive way, but for now, being conservative in CSSValueConversion like this works.

Sure, I'll update it. I want to confirm my understanding of what this actually does. Essentially the setCurrentPropertyInvalidAtComputedValueTime will make sure that the overflow-clip-margin's computed value is set to 0px if it's invalid.

Not quite. When setCurrentPropertyInvalidAtComputedValueTime() is used, it sets a bit in the style builder that tells it that no matter what value ends up being set, it should be unset. If you look in Style::Builder::applyProperty(), at the end of the function, there is logic that checks isCurrentPropertyInvalidAtComputedValueTime() and applies the unset.

We usually return the default value for the property when we call setCurrentPropertyInvalidAtComputedValueTime() just for simplicity. It doesn't really matter what is returned in those cases.

Take for example StyleBorderRadius.cpp. Why is setCurrentPropertyInvalidAtComputedValueTime not called here - https://searchfox.org/wubkat/source/Source/WebCore/style/values/borders/StyleBorderRadius.cpp#71 ?

I think that check for if (!value.isPair()) is probably unnecessary and wrong, since the requiredPairDowncast will perform the same check.

And for StyleWebKitTextStrokeWidth.cpp, it's only called once in line 58 but not in line 42 -
https://searchfox.org/wubkat/source/Source/WebCore/style/values/non-standard/StyleWebKitTextStrokeWidth.cpp#38. Why is that the case?

It's not necessary to call it for line 42 because that is what the requiredDowncast() above it did. requiredDowncast is a dynamicDowncast that in addition to checking the type, calls setCurrentPropertyInvalidAtComputedValueTime() if the type check fails.

We need to do it on line 58 because we are narrowing our check beyond just CSSPrimitiveValue, and we don't have a specific "required" function for that case.

Ah, I get it now. Thanks for the clear explanation.

This can be written a bit more succinctly using an early return and a more specific subtype.

That's a lot cleaner. I'll update it.

In cases like these, we allow the values to be in any order. So this could be written as:

std::optional<OverflowClipReferenceBox> referenceBox;
std::optional<OverflowClipMargin::Length> length;

for (Ref item : *list) {
    if (item->isValueID()) {
        if (referenceBox) {
            state.setCurrentPropertyInvalidAtComputedValueTime();
            return 0_css_px;
        }
        referenceBox = toStyleFromCSSValue<OverflowClipReferenceBox>(state, item);
    } else if (item->isLength()) {
        if (length) {
            state.setCurrentPropertyInvalidAtComputedValueTime();
            return 0_css_px;
        }
        length = toStyleFromCSSValue<OverflowClipMargin::Length>(state, item);
    } else {
        state.setCurrentPropertyInvalidAtComputedValueTime();
        return 0_css_px;
    }
}

return { *referenceBox, *length };

Sure I'll update it. I'm curious as to when the value order is reversed. The auto generated parser makes sure that the CSSValueList always has referenceBox as first value. In what situation would the CSSValueList have length as the first value?

I'd have to dive deeper to find out if there is a way to get it to happen, but the short answer is that the CSS typedom (e.g. StylePropertyMap) can also be the source of the CSSValue passed to CSSValueConversion. For a case like this, there would have to be another property had a similar (but with the reverse default ordering) grammar, and allowed <length> <visual-box>. If such a property existed, one could do something like:

      var element = document.getElementById("element");
      element.attributeStyleMap.set('overflow-clip-margin', CSSStyleValue.parse('made-up-property', '30px border-box'))

As a result of this other source, we choose to be conservative in the CSSValueConversion codepath.

Got it.

You probably don't want to do this. Even if the behavior will end up being that it should act like a length of 0px is set, the resolved value (that is what the value returned from getComputedStyle is called) will still need to be padding-box.

Yup that makes a lot of sense. I realized later that this code has nothing to do with CSS value canonicalization.

Our convention is put each of these elements in the array on their own line.

I'll update it.

I would put the full grammar here. <box> isn't even in a spec anymore, so others should probably be using <visual-box> going forward.

Please remove this newline.

Please add a newline under this.

CSSBoxType is really too broad for this. I would stick with adding a new VisualBox type.

Sure.

You are going to need more constructors. If you want to leave that for the next change, that is fine.

I'll leave that for the next change.

I would move this to be in the body of CSSValueConversion::operator() to constrain it.

Is this include needed?

These are not needed. I'll remove them.

Is this include needed?

Nope. I'll remove them.

You need to check that at least one of the values got parsed here.

if (!visualBox && !length)
    return nullptr;

This is the code that's below the the for loop

CSSValueListBuilder list;
// Default value is padding-box
if (visualBox && !isValueID(visualBox, CSSValueID::CSSValuePaddingBox))
    list.append(visualBox.releaseNonNull());

// Default value is 0px
if (length && !length->isZero().value_or(true))
    list.append(length.releaseNonNull());

if (list.isEmpty())
    return { CSSPrimitiveValue::create(0, CSSUnitType::CSS_PX) };

Isn't the list.isEmpty() sufficient to check that at least one of the values got parsed? Or is there a specific reason we want return a nullptr when both of them empty?

I realized later that we need this. I added it in the new revision.

Why is value_or(true) right here?

(Note, isZero() will return nullopt when it can't be determined if the value is zero, like when calc() is used).

I initially wrote value_or(true) because if length is empty then we would not want to add it to the list. But now I realize that it won't work when calc() is used. It should be value_or(false). I'll change it.

small nit to match the style of the others

Sure, I'll update the text.