Bug 223252 – [css-contain] Parse CSS contain property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Parse CSS contain property.

[Rob Buis]

Created attachment 423325 [details]
Patch

[Rob Buis]

Created attachment 423381 [details]
Patch

[Rob Buis]

Created attachment 423481 [details]
Patch

[Rob Buis]

Created attachment 423585 [details]
Patch

[Rob Buis]

Created attachment 423592 [details]
Patch

[Rob Buis]

Created attachment 423618 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 423618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423618&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3486
> +        case CSSPropertyContain: {
> +            auto containment = style.contain();

Should there be a feature enabled check here? Maybe add a test that contain is not exposed in compute style when disabled.

> Source/WebCore/rendering/style/RenderStyleConstants.h:1212
> +    None     = 0,

Generally this would get used in an OptionSet<> so no need to define the None value.

> Source/WebCore/rendering/style/RenderStyleConstants.h:1218
> +    Strict   = Containment::Layout | Containment::Paint | Containment::Size,
> +    Content  = Containment::Layout | Containment::Paint,

These should be predefined OptionSet<>s.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
> +    unsigned contain;

Use an OptionSet<>?

> Source/WebCore/style/StyleBuilderCustom.h:1250
> +    if (size)
> +        contain |= static_cast<unsigned>(Containment::Size);
> +    if (layout)
> +        contain |= static_cast<unsigned>(Containment::Layout);
> +    if (style)
> +        contain |= static_cast<unsigned>(Containment::Style);
> +    if (paint)
> +        contain |= static_cast<unsigned>(Containment::Paint);

OptionSet<> makes this cleaner.

[Darin Adler]

Comment on attachment 423618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423618&action=review

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
>> +    unsigned contain;
> 
> Use an OptionSet<>?

Also should just use the 8 bits instead of 32.

[Darin Adler]

Using more OptionSet is going to make this much nicer.

[Rob Buis]

Created attachment 423721 [details]
Patch

[Rob Buis]

Comment on attachment 423618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423618&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3486
>> +            auto containment = style.contain();
> 
> Should there be a feature enabled check here? Maybe add a test that contain is not exposed in compute style when disabled.

You are right, I added the check and the test contain-invalidate-if-disabled.html.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:1212
>> +    None     = 0,
> 
> Generally this would get used in an OptionSet<> so no need to define the None value.

Removed.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:1218
>> +    Content  = Containment::Layout | Containment::Paint,
> 
> These should be predefined OptionSet<>s.

Done.

>>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
>>> +    unsigned contain;
>> 
>> Use an OptionSet<>?
> 
> Also should just use the 8 bits instead of 32.

Done.

>> Source/WebCore/style/StyleBuilderCustom.h:1250
>> +        contain |= static_cast<unsigned>(Containment::Paint);
> 
> OptionSet<> makes this cleaner.

Agreed, done.

[Darin Adler]

Comment on attachment 423721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423721&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3487
> +            if (renderer && !renderer->settings().cssContainmentEnabled())
> +                return nullptr;

It’s not right to use the renderer to get to settings. We need this to get the correct settings when there is no renderer. I think we should use m_element.

    if (m_element && ! m_element->document().settings().cssContainmentEnabled())

I think the same mistake is repeated in many other places in this function, and can likely be tested by using computed style APIs for non-rendered elements.

> Source/WebCore/style/StyleBuilderCustom.h:1219
> +        if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone)
> +            return builderState.style().setContain(RenderStyle::initialContainment());
> +        if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueStrict)
> +            return builderState.style().setContain(RenderStyle::strictContainment());

Why not put the valueID into a local variable?

Consider switch instead of cascading if statements.

> Source/WebCore/style/StyleBuilderCustom.h:1229
> +        CSSPrimitiveValue& value = downcast<CSSPrimitiveValue>(item.get());

auto&

But also, why not have the local variable be the valueID instead of the CSSPrimitiveValue object?

> Source/WebCore/style/StyleBuilderCustom.h:1230
> +        if (value.valueID() == CSSValueSize)

Consider switch instead of cascading if statements.

[Rob Buis]

Created attachment 423862 [details]
Patch

[Rob Buis]

Created attachment 423884 [details]
Patch

[Darin Adler]

Comment on attachment 423884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423884&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3486
> +            if (renderer && !renderer->settings().cssContainmentEnabled())

This is still using the renderer. I haven’t seen a response about my comment about using renderer to get to settings.

[Rob Buis]

Comment on attachment 423884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423884&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3486
>> +            if (renderer && !renderer->settings().cssContainmentEnabled())
> 
> This is still using the renderer. I haven’t seen a response about my comment about using renderer to get to settings.

I was waiting for the repo to re-open :) I think this should be another bug, since there are several of these instances, not just CSS contain.

[Darin Adler]

Sounds reasonable to deal with that in a separate bug. If you had said that, and opened the bug, then I would have understood your plan!

[Rob Buis]

(In reply to Darin Adler from comment #17)
> Sounds reasonable to deal with that in a separate bug. If you had said that,
> and opened the bug, then I would have understood your plan!

Fair enough :) I opened https://bugs.webkit.org/show_bug.cgi?id=223598 for this.

[Rob Buis]

Comment on attachment 423721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423721&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3487
>> +                return nullptr;
> 
> It’s not right to use the renderer to get to settings. We need this to get the correct settings when there is no renderer. I think we should use m_element.
> 
>     if (m_element && ! m_element->document().settings().cssContainmentEnabled())
> 
> I think the same mistake is repeated in many other places in this function, and can likely be tested by using computed style APIs for non-rendered elements.

I opened https://bugs.webkit.org/show_bug.cgi?id=223598 for this.

>> Source/WebCore/style/StyleBuilderCustom.h:1219
>> +            return builderState.style().setContain(RenderStyle::strictContainment());
> 
> Why not put the valueID into a local variable?
> 
> Consider switch instead of cascading if statements.

Done.

>> Source/WebCore/style/StyleBuilderCustom.h:1229
>> +        CSSPrimitiveValue& value = downcast<CSSPrimitiveValue>(item.get());
> 
> auto&
> 
> But also, why not have the local variable be the valueID instead of the CSSPrimitiveValue object?

Done.

>> Source/WebCore/style/StyleBuilderCustom.h:1230
>> +        if (value.valueID() == CSSValueSize)
> 
> Consider switch instead of cascading if statements.

Done.

[EWS]

Committed r274793: <https://commits.webkit.org/r274793>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423884 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75707365>

[Sam Weinig]

Comment on attachment 423884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423884&action=review

Sorry for the late post land review, but I noticed a few things.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3504
> +            if (!containment)
> +                return cssValuePool.createIdentifierValue(CSSValueNone);
> +            if (containment == RenderStyle::strictContainment())
> +                return cssValuePool.createIdentifierValue(CSSValueStrict);
> +            if (containment == RenderStyle::contentContainment())
> +                return cssValuePool.createIdentifierValue(CSSValueContent);
> +            auto list = CSSValueList::createSpaceSeparated();
> +            if (containment & Containment::Size)
> +                list->append(cssValuePool.createIdentifierValue(CSSValueSize));
> +            if (containment & Containment::Layout)
> +                list->append(cssValuePool.createIdentifierValue(CSSValueLayout));
> +            if (containment & Containment::Style)
> +                list->append(cssValuePool.createIdentifierValue(CSSValueStyle));
> +            if (containment & Containment::Paint)
> +                list->append(cssValuePool.createIdentifierValue(CSSValuePaint));
> +            return list;

This doesn't seem to match the current version of the spec which states: "the keyword none or one or more of size, layout, paint" - https://drafts.csswg.org/css-contain-1/#propdef-contain

> Source/WebCore/css/CSSValueKeywords.in:1526
> +// style

I don't see "style" in the current spec - https://drafts.csswg.org/css-contain-1/#propdef-contain. Is it from something else perhaps?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3933
> +    RefPtr<CSSPrimitiveValue> singleValue;
> +    if (range.peek().type() == IdentToken)
> +        singleValue = consumeIdent<CSSValueNone, CSSValueStrict, CSSValueContent>(range);
> +    if (singleValue)
> +        return singleValue;

I think this can all be collapsed into:

if (auto singleValue = consumeIdent<<CSSValueNone, CSSValueStrict, CSSValueContent>(range))
    return singleValue;

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3947
> +        auto id = range.peek().id();
> +        if (id == CSSValueSize && !size)
> +            size = consumeIdent(range);
> +        else if (id == CSSValueLayout && !layout)
> +            layout = consumeIdent(range);
> +        else if (id == CSSValueStyle && !style)
> +            style = consumeIdent(range);
> +        else if (id == CSSValuePaint && !paint)
> +            paint = consumeIdent(range);
> +        else
> +            break;

We usually do this something like:

switch (range.peek().id()) {
    case CSSValueSize:
        if (size)
            return nullptr; // I think this is right, but the CSS grammar rules trip me up sometimes. Does || allow for an element to appear more than once?
        size = consumeIdent(range);
        break;

      ...

    default:
        return nullptr;

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 424114 [details]
Patch

[Rob Buis]

Comment on attachment 423884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423884&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3504
>> +            return list;
> 
> This doesn't seem to match the current version of the spec which states: "the keyword none or one or more of size, layout, paint" - https://drafts.csswg.org/css-contain-1/#propdef-contain

Right, it was probably a mistake to add style, since it is at-risk. Removed it for now until it has a more clear future path forward.
Note that content/strict need to be serialized like this because of the Candidate correction 1 (https://drafts.csswg.org/css-contain-1/#contain-property).

>> Source/WebCore/css/CSSValueKeywords.in:1526
>> +// style
> 
> I don't see "style" in the current spec - https://drafts.csswg.org/css-contain-1/#propdef-contain. Is it from something else perhaps?

See above.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3933
>> +        return singleValue;
> 
> I think this can all be collapsed into:
> 
> if (auto singleValue = consumeIdent<<CSSValueNone, CSSValueStrict, CSSValueContent>(range))
>     return singleValue;

Nice! Fixed.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:3947
>> +            break;
> 
> We usually do this something like:
> 
> switch (range.peek().id()) {
>     case CSSValueSize:
>         if (size)
>             return nullptr; // I think this is right, but the CSS grammar rules trip me up sometimes. Does || allow for an element to appear more than once?
>         size = consumeIdent(range);
>         break;
> 
>       ...
> 
>     default:
>         return nullptr;

Yeah, better, done.

[Sam Weinig]

(In reply to Rob Buis from comment #25)
> Comment on attachment 423884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423884&action=review
> 
> >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3504
> >> +            return list;
> > 
> > This doesn't seem to match the current version of the spec which states: "the keyword none or one or more of size, layout, paint" - https://drafts.csswg.org/css-contain-1/#propdef-contain
> 
> Right, it was probably a mistake to add style, since it is at-risk. Removed
> it for now until it has a more clear future path forward.
> Note that content/strict need to be serialized like this because of the
> Candidate correction 1
> (https://drafts.csswg.org/css-contain-1/#contain-property).
> 

The issue I was intending to point out was that that the computed style should not ever be "strict" or "content", which it looks like it still can be here.

[Sam Weinig]

(In reply to Sam Weinig from comment #26)
> (In reply to Rob Buis from comment #25)
> > Comment on attachment 423884 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423884&action=review
> > 
> > >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3504
> > >> +            return list;
> > > 
> > > This doesn't seem to match the current version of the spec which states: "the keyword none or one or more of size, layout, paint" - https://drafts.csswg.org/css-contain-1/#propdef-contain
> > 
> > Right, it was probably a mistake to add style, since it is at-risk. Removed
> > it for now until it has a more clear future path forward.
> > Note that content/strict need to be serialized like this because of the
> > Candidate correction 1
> > (https://drafts.csswg.org/css-contain-1/#contain-property).
> > 
> 
> The issue I was intending to point out was that that the computed style
> should not ever be "strict" or "content", which it looks like it still can
> be here.

Oh, hm, I think I just don't understand the way this is being done. The buttons are confusing.

[Rob Buis]

(In reply to Sam Weinig from comment #27)
> > The issue I was intending to point out was that that the computed style
> > should not ever be "strict" or "content", which it looks like it still can
> > be here.
> 
> Oh, hm, I think I just don't understand the way this is being done. The
> buttons are confusing.

I think the buttons are okay and a nice touch. I interpret the correction as "do not bother keeping state about whether this was specified as strict or content, it will be serialized to that either way", which we do. Given the shortest-serialization principle, I think we have to do this code anyway, and it passes the tests and behaves like other implementations. Let me know if you suggest something different for this particular code.

[Sam Weinig]

(In reply to Rob Buis from comment #28)
> (In reply to Sam Weinig from comment #27)
> > > The issue I was intending to point out was that that the computed style
> > > should not ever be "strict" or "content", which it looks like it still can
> > > be here.
> > 
> > Oh, hm, I think I just don't understand the way this is being done. The
> > buttons are confusing.
> 
> I think the buttons are okay and a nice touch. I interpret the correction as
> "do not bother keeping state about whether this was specified as strict or
> content, it will be serialized to that either way", which we do. Given the
> shortest-serialization principle, I think we have to do this code anyway,
> and it passes the tests and behaves like other implementations. Let me know
> if you suggest something different for this particular code.

Nah, seems fine for now. If the tests change, we will notice it.

I do think it would be good to have some tests of parse failure (e.g. paint paint, etc.).

[EWS]

Committed r274957: <https://commits.webkit.org/r274957>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424114 [details].

[Rob Buis]

(In reply to Sam Weinig from comment #29)
> I do think it would be good to have some tests of parse failure (e.g. paint
> paint, etc.).

Yeah good point, I will try to update the relevant WPT test at some point.