Bug 223252 - [css-contain] Parse CSS contain property
Summary: [css-contain] Parse CSS contain property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 172026
  Show dependency treegraph
 
Reported: 2021-03-16 07:19 PDT by Rob Buis
Modified: 2021-03-24 12:28 PDT (History)
15 users (show)

See Also:


Attachments
Patch (13.99 KB, patch)
2021-03-16 07:21 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (35.42 KB, patch)
2021-03-16 12:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (46.45 KB, patch)
2021-03-17 06:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (52.92 KB, patch)
2021-03-18 04:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (52.99 KB, patch)
2021-03-18 06:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (115.16 KB, patch)
2021-03-18 10:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (115.45 KB, patch)
2021-03-19 06:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (115.68 KB, patch)
2021-03-22 01:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (115.75 KB, patch)
2021-03-22 08:19 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2021-03-24 02:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-03-16 07:19:57 PDT
Parse CSS contain property.
Comment 1 Rob Buis 2021-03-16 07:21:52 PDT
Created attachment 423325 [details]
Patch
Comment 2 Rob Buis 2021-03-16 12:40:35 PDT
Created attachment 423381 [details]
Patch
Comment 3 Rob Buis 2021-03-17 06:45:19 PDT
Created attachment 423481 [details]
Patch
Comment 4 Rob Buis 2021-03-18 04:28:56 PDT
Created attachment 423585 [details]
Patch
Comment 5 Rob Buis 2021-03-18 06:39:06 PDT
Created attachment 423592 [details]
Patch
Comment 6 Rob Buis 2021-03-18 10:59:10 PDT
Created attachment 423618 [details]
Patch
Comment 7 Simon Fraser (smfr) 2021-03-18 12:03:35 PDT
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.
Comment 8 Darin Adler 2021-03-18 15:32:50 PDT
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.
Comment 9 Darin Adler 2021-03-18 15:33:10 PDT
Using more OptionSet is going to make this much nicer.
Comment 10 Rob Buis 2021-03-19 06:16:41 PDT
Created attachment 423721 [details]
Patch
Comment 11 Rob Buis 2021-03-19 08:54:59 PDT
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.
Comment 12 Darin Adler 2021-03-19 12:43:44 PDT
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.
Comment 13 Rob Buis 2021-03-22 01:36:19 PDT
Created attachment 423862 [details]
Patch
Comment 14 Rob Buis 2021-03-22 08:19:02 PDT
Created attachment 423884 [details]
Patch
Comment 15 Darin Adler 2021-03-22 11:59:22 PDT
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.
Comment 16 Rob Buis 2021-03-22 12:03:47 PDT
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.
Comment 17 Darin Adler 2021-03-22 12:18:59 PDT
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!
Comment 18 Rob Buis 2021-03-22 13:28:38 PDT
(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.
Comment 19 Rob Buis 2021-03-22 13:30:00 PDT
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.
Comment 20 EWS 2021-03-22 13:51:16 PDT
Committed r274793: <https://commits.webkit.org/r274793>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423884 [details].
Comment 21 Radar WebKit Bug Importer 2021-03-22 13:52:35 PDT
<rdar://problem/75707365>
Comment 22 Sam Weinig 2021-03-23 16:53:43 PDT
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;
Comment 23 Rob Buis 2021-03-24 02:43:50 PDT
Reopening to attach new patch.
Comment 24 Rob Buis 2021-03-24 02:43:54 PDT
Created attachment 424114 [details]
Patch
Comment 25 Rob Buis 2021-03-24 07:31:12 PDT
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.
Comment 26 Sam Weinig 2021-03-24 10:29:58 PDT
(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.
Comment 27 Sam Weinig 2021-03-24 10:31:27 PDT
(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.
Comment 28 Rob Buis 2021-03-24 11:12:31 PDT
(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.
Comment 29 Sam Weinig 2021-03-24 11:24:38 PDT
(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.).
Comment 30 EWS 2021-03-24 12:10:13 PDT
Committed r274957: <https://commits.webkit.org/r274957>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424114 [details].
Comment 31 Rob Buis 2021-03-24 12:28:06 PDT
(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.