Bug 217802 – Unify CSS property enablement (-apple-color-filter should not exist on CSSStyleDeclaration by default)

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

[Sam Sneddon [:gsnedders]]

per smfr in https://bugs.webkit.org/show_bug.cgi?id=217626#c1

[Radar WebKit Bug Importer]

<rdar://problem/70600812>

[Sam Sneddon [:gsnedders]]

Further investigation shows that this is only the attribute on CSSStyleDeclaration which is exposed; the CSS parser special-cases it.

Looking at https://wpt.fyi/results/css/css-conditional/js/CSS-supports-CSSStyleDeclaration.html?label=master&label=experimental&aligned&q=safari%3Afail shows there's a few more: overscroll-behavior{,-x,-y} and scroll-behavior. (The other failures are other issues)

[Sam Sneddon [:gsnedders]]

Bug 217623 fixed most of these, but there's a few still failing.

[Sam Sneddon [:gsnedders]]

So properties that look sus now:

-apple-color-filter
-webkit-text-size-adjust

There's also the failures due to Bug 224930.

[Sam Sneddon [:gsnedders]]

Created attachment 427391 [details]
Patch

By no means done, but maybe getting somewhere useful?

[Sam Sneddon [:gsnedders]]

Created attachment 427435 [details]
Patch

Still not quite there, but would be interested in some initial review at least. (Things can be better, for sure!)

[Darin Adler]

Comment on attachment 427435 [details]
Patch

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

This looks like a great approach; brief look at the patch doesn’t make it clear to me how many different bits there are in CSSPropertySettings.

> Source/WebCore/css/makeprop.pl:556
> +foreach my $name (uniq(values %settingsFlags)) {

Any sorting needed for predictable order?

> Source/WebCore/css/makeprop.pl:715
> +    inline CSSPropertySettings() {};

The word "inline" here isn’t needed. The semicolon isn’t needed. But I suggest this alternative form:

    CSSPropertySetting() = default;

> Source/WebCore/css/makeprop.pl:716
> +    CSSPropertySettings(const Settings& settings);

WebKit coding style says leave out the argument name here. I also suggest "explicit" for this.

> Source/WebCore/css/makeprop.pl:717
> +    bool isPropertyEnabled(const CSSPropertyID id) const;

No need for the "const" here or the "id". Just:

    bool isPropertyEnabled(CSSPropertyID) const;

> Source/WebCore/css/makeprop.pl:725
> +void add(Hasher&, const CSSPropertySettings&);
> +
> +
>  } // namespace WebCore

Just one blank line.

[Sam Sneddon [:gsnedders]]

Created attachment 427671 [details]
List of calls to isCSSPropertyEnabledBySettings/isInternalCSSProperty etc.

[Sam Sneddon [:gsnedders]]

Looking through this new attachment makes it very clear that the vast majority of the time we're calling things like:

> if (!isCSSPropertyEnabledBySettings(propertyID, &m_element->document().settings()) || !isEnabledCSSProperty(propertyID) || isInternalCSSProperty(propertyID))

This is… rather silly. It almost makes me wonder how many of the other cases are actually wrong.

I wonder if we should just have a single function in CSSPropertyNames.h called something like isCSSPropertyExposed(CSSPropertyID, Settings&)?

[Darin Adler]

(In reply to Sam Sneddon [:gsnedders] from comment #9)
> I wonder if we should just have a single function in CSSPropertyNames.h
> called something like isCSSPropertyExposed(CSSPropertyID, Settings&)?

We should add this, then see how many call sites need something more specific. We can remove the other functions once we confirm they aren’t needed.

[Sam Sneddon [:gsnedders]]

Created attachment 427753 [details]
Patch

[Sam Sneddon [:gsnedders]]

Comment on attachment 427753 [details]
Patch

It turns out uploading the wrong set of commits isn't helpful.

[Sam Sneddon [:gsnedders]]

Created attachment 427807 [details]
Patch

[Darin Adler]

Comment on attachment 427807 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1472
> +        CSSPropertyID propertyID = computedPropertyIDs[i];

I prefer auto here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1475
> +        auto name = getPropertyName(propertyID);

Not sure we need to stick this into a local; OK, I guess, but could also just call it on the append line like we did before.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1478
> +            // FIXME: ideally we'd assert in this case, but currently it's hit too often

WebKit coding style says write this like a sentence with a capitalized first word and a full stop at the end (which I call a period because educated in U.S.).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3997
> +        /* Internal properties should be handled by isCSSPropertyExposed above */

WebKit coding style encourages use of C++ style // comments, even if code nearby was done in a different style (not sure why it was, maybe imported from Chromium).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4223
> +        auto propertyID = set[i];

While this probably how I would write it, there are others who discourage me from caching simply computed values like this in local variables.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4225
> +            list.append(CSSProperty(propertyID, WTFMove(value), false));

After doing reserveInitialCapacity, can and should use uncheckedAppend instead of append for a little tighter code. Not sure why the old code was not doing that.

> Source/WebCore/css/CSSStyleDeclaration.cpp:203
> -    if (!isEnabledCSSProperty(id) || !isCSSPropertyEnabledBySettings(id, settings))
> +    if (!isCSSPropertyExposed(id, settings))

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/DOMCSSNamespace.cpp:64
> -    if (parserContext.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &document.settings()))

Ditto.

> Source/WebCore/css/StyleProperties.cpp:863
> +    if (!isCSSPropertyExposed(propertyID, &parserContext.propertySettings) && !isInternalCSSProperty(propertyID)) {

I sort of expected a single helper function call here, not two. What makes this different enough from the other call sites that it needs an explicit isInternalCSSProperty check and they do not?

> Source/WebCore/css/makeprop.pl:337
> +static bool isEnabledCSSProperty(const CSSPropertyID);
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const Settings*);
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID, const CSSPropertySettings*);

These should all say "CSSPropertyID", not "const CSSPropertyID".

But also, why do we need to declare them at the top of the file? Why not just define them?

> Source/WebCore/css/makeprop.pl:404
> +static bool isEnabledCSSProperty(const CSSPropertyID id)

Ditto.

> Source/WebCore/css/makeprop.pl:419
> +static bool isEnabledCSSProperty(const CSSPropertyID)

Ditto.

> Source/WebCore/css/makeprop.pl:428
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID id, const Settings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:448
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID id, const CSSPropertySettings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:465
> +    return true;

This isn’t needed, because we have the default above.

> Source/WebCore/css/makeprop.pl:468
> +bool isCSSPropertyExposed(const CSSPropertyID id, const Settings* settings)

Ditto, no const CSSPropertyID.

> Source/WebCore/css/makeprop.pl:473
> +bool isCSSPropertyExposed(const CSSPropertyID id, const CSSPropertySettings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:732
>  bool isInternalCSSProperty(const CSSPropertyID);
> -bool isEnabledCSSProperty(const CSSPropertyID);
> -bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const Settings* = nullptr);
> +bool isCSSPropertyExposed(const CSSPropertyID, const Settings*);
> +bool isCSSPropertyExposed(const CSSPropertyID, const CSSPropertySettings*);

Same here, just CSSPropertyID, no const.

> Source/WebCore/css/makeprop.pl:735
>  const WTF::AtomString& getPropertyNameAtomString(CSSPropertyID id);
>  WTF::String getPropertyNameString(CSSPropertyID id);

Should take these "id" out of here.

> Source/WebCore/css/parser/CSSParserContext.cpp:87
> +    , propertySettings { CSSPropertySettings(document.settings()) }

Could use braces instead of parentheses here if you like.

    , propertySettings { CSSPropertySettings { document.settings() } }

> Source/WebCore/css/parser/CSSParserImpl.cpp:813
> -    if (m_context.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &m_context.propertySettings))

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:114
>          auto propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);
> -        // FIXME: Should take account for flags in settings().
> -        if (isEnabledCSSProperty(propertyID))
> -            return propertyID;
> +        return propertyID;

I do not think we need a local variable here or braces.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:204
> -    if (!isEnabledCSSProperty(property))
> +    if (!isCSSPropertyExposed(property, &m_context.propertySettings) && !isInternalCSSProperty(property)) {

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:278
> +    if (!isCSSPropertyExposed(property, &context.propertySettings)) {
> +        ASSERT_NOT_REACHED();
> +        return nullptr;
> +    }

Is it important to add the check here? All these new places we are checking ... are they detectable? Is the benefit substantial enough to justify additional runtime overhead.

Maybe just an assertion instead of a always-compiled check to back the assertion. I’m concerned about adding all these additional checks just to be assertions.

[Sam Sneddon [:gsnedders]]

Created attachment 447781 [details]
Patch

[Sam Sneddon [:gsnedders]]

Not putting this up for review quite yet; could still do with slightly better testing (though this is gonna run into CSSOM being underdefined…).

[Devin Rousso]

Comment on attachment 447781 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.h:178
> +    const RefPtr<Settings> m_settings;

Rather than hold the `Settings`, could we just have a `Page& m_inspectedPage` instead?  This is what many of the other `Inspector*Agent` do so it would be nice to match that pattern :)

> LayoutTests/inspector/css/css-property.html:49
> +                    case "background-repeat-y":

Is this an actual legit property now?  If not, can you explain why this was changed?

[Sam Sneddon [:gsnedders]]

Created attachment 456127 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Tim Nguyen (:ntim)]

Comment on attachment 456127 [details]
Patch

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

> Source/WebCore/ChangeLog:87
> [phone]  Sam Sneddon  <[e-mail]>
> +
> +        -apple-color-filter should not exist on CSSStyleDeclaration by default
> +        https://bugs.webkit.org/show_bug.cgi?id=217802
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Double changelog

[Darin Adler]

Comment on attachment 456127 [details]
Patch

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

Looks like a really good approach; tests not passing yet though

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4333
>          auto results = copyToVector(inheritedCustomProperties.keys());
> -        return results.at(i - numComputedPropertyIDs);
> +        return results.at(i - m_activeComputedPropertyIDs.size());

Not new to this patch: This seems so inefficient; there must be some way to get the nth without building a vector. Also, is it really OK to have the order depend on hash table ordering? Seems like we’d want things to be in a predictable order, not pseudo-random.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4338
>      auto results = copyToVector(nonInheritedCustomProperties.keys());
> -    return results.at(i - inheritedCustomProperties.size() - numComputedPropertyIDs);
> +    return results.at(i - inheritedCustomProperties.size() - m_activeComputedPropertyIDs.size());

Ditto.

[Darin Adler]

Wow I did not remember the last time I reviewed this, 10 months ago.

Maybe we should break this down into a set of smaller changes so we can make some progress on it. Would still like it to end up just like this.

[Sam Sneddon [:gsnedders]]

(In reply to Darin Adler from comment #22)
> Wow I did not remember the last time I reviewed this, 10 months ago.
> 
> Maybe we should break this down into a set of smaller changes so we can make
> some progress on it. Would still like it to end up just like this.

I basically just need to make sure the per-platform expectations are right, I think otherwise we're pretty much good to go, modulo any further review comments, which I'd been hoping to get done before I was out at the end of last week. This week, hopefully!

[Sam Sneddon [:gsnedders]]

Comment on attachment 447781 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.h:178
>> +    const RefPtr<Settings> m_settings;
> 
> Rather than hold the `Settings`, could we just have a `Page& m_inspectedPage` instead?  This is what many of the other `Inspector*Agent` do so it would be nice to match that pattern :)

👍

>> LayoutTests/inspector/css/css-property.html:49
>> +                    case "background-repeat-y":
> 
> Is this an actual legit property now?  If not, can you explain why this was changed?

It's not, it's the opposite: it's no longer accidentally allowed in some places, as it's an internal property. The old results are clearly nonsense: it should always be the same as the equally invalid background-repeat-x and background-repeat-invalid.

[Sam Sneddon [:gsnedders]]

Created attachment 456722 [details]
Patch

I'm hoping I now have all the per-platform expectations accurate and this can land, but EWS alone will say.

[Sam Sneddon [:gsnedders]]

Unfortunately, still regressed:

imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/computed/computed.tentative.html
imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/computed/iterable.tentative.html

[Sam Sneddon [:gsnedders]]

Created attachment 456964 [details]
Patch

[Darin Adler]

Comment on attachment 456964 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
> +    for (auto & propertyID : computedPropertyIDs) {

Formatting mistake. This should be "auto&" with no space in WebKit coding style.

Is computedPropertyIDs ordered? Seems this ordering may be web-visible?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1694
> +        if (isCSSPropertyExposed(propertyID, &m_element->document().settings()))

Could hoist the m_element->document().settings() out of the loop to avoid chasing the same pointer repeatedly.

Is there some faster way to do this across the properties? I imagine there are some repeated operations we do when calling isCSSPropertyExposed over and over again.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
> +    m_activeComputedPropertyIDs = WTFMove(active);

Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:352
> +    return ::WebCore::isCSSPropertyExposed(propertyID, &parserContext.propertySettings)

We’d normally write "WebCore::" not "::WebCore::" even though you could do either.

> Source/WebCore/css/parser/CSSParserContext.cpp:170
> +        | (unsigned long long)context.mode                  << 21; // This is multiple bits, so keep it last.

The typecast should be uint64_t since that’s the type we use above for the whole thing.

By the way, do we really need 64 bits? Is 12 bits not enough for context.mode? Because, if it is, we can use uint32_t instead of uint64_t for "bits" and that’s going to be more efficient.

> Source/WebCore/css/parser/CSSParserContext.h:39
> +class Settings;

I don’t understand this change. I don’t see any new uses of Settings below in changes to this header file, although I see a new use of CSSPropertySettings.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:123
>      if (auto hashTableEntry = findProperty(buffer, length)) {
> -        auto propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);
> -        // FIXME: Should take account for flags in settings().
> -        if (isEnabledCSSProperty(propertyID))
> -            return propertyID;
> +        return static_cast<CSSPropertyID>(hashTableEntry->id);
>      }

WebKit coding style says no braces for a one line if body like this one.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4945
> +    ASSERT(context.propertySettings.cssCounterStyleAtRulesEnabled && isCSSPropertyExposed(propId, &context.propertySettings));

I typically suggest two separate ASSERT rather than an ASSERT with && in it, in part because it can make it clearer which of the two failed.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
> +    Vector<CSSPropertyID> active;
> +    active.reserveInitialCapacity(numComputedPropertyIDs);
> +    for (auto & propertyID : computedPropertyIDs) {
> +        if (isCSSPropertyExposed(propertyID, &element.document().settings()))
> +            active.uncheckedAppend(propertyID);
> +    }
> +    active.shrinkToFit();
> +
> +    m_activeComputedPropertyIDs = WTFMove(active);

Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:127
> +    for (auto & propertyID : m_activeComputedPropertyIDs) {

Given that property IDs are simple scalars, I suggest just "auto" here instead of "auto&". (Also WebKit coding style says "auto&" without the space.)

> Source/WebCore/inspector/agents/InspectorCSSAgent.h:66
> +    InspectorCSSAgent(PageAgentContext&);

This line of code really needs "explicit" (not new).

[Sam Sneddon [:gsnedders]]

Created attachment 456977 [details]
Patch

[Sam Sneddon [:gsnedders]]

(This new patch only fixes the merge conflict, doesn't actually address anything else, such as from Darin's review above. It should find any new platform-specific failures on EWS at least…)

[Darin Adler]

Comment on attachment 456964 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>> +    for (auto & propertyID : computedPropertyIDs) {
> 
> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
> 
> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?

Oh, actually just auto, realized this is a scalar.

[Sam Sneddon [:gsnedders]]

Comment on attachment 456964 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>>> +    for (auto & propertyID : computedPropertyIDs) {
>> 
>> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
>> 
>> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?
> 
> Oh, actually just auto, realized this is a scalar.

It's ordered (alphabetically, with prefixed properties after unprefixed ones). Is there any concern about the ordering here? It should be preserved throughout, I think?

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1694
>> +        if (isCSSPropertyExposed(propertyID, &m_element->document().settings()))
> 
> Could hoist the m_element->document().settings() out of the loop to avoid chasing the same pointer repeatedly.
> 
> Is there some faster way to do this across the properties? I imagine there are some repeated operations we do when calling isCSSPropertyExposed over and over again.

After a small amount of inlining, it's just a switch statement with things like:

    case CSSPropertyID::CSSPropertyRotate:
        return settings->cssIndividualTransformPropertiesEnabled;

It's not really clear there's anything we can do better than calling it repeatedly.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
>> +    m_activeComputedPropertyIDs = WTFMove(active);
> 
> Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.

Perhaps a silly question: what's the easiest way to stack-allocate something to build it before moving it into a FixedVector?

>> Source/WebCore/css/parser/CSSParserContext.h:39
>> +class Settings;
> 
> I don’t understand this change. I don’t see any new uses of Settings below in changes to this header file, although I see a new use of CSSPropertySettings.

This'll be me failing to remove it from earlier iterations of the patch!

>> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
>> +    m_activeComputedPropertyIDs = WTFMove(active);
> 
> Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.

I couldn't figure out where I ought to put this; any suggestions? I duplicated it mostly to reach a point where I could get tests passing!

[Darin Adler]

Comment on attachment 456964 [details]
Patch

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

>>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>>>> +    for (auto & propertyID : computedPropertyIDs) {
>>> 
>>> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
>>> 
>>> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?
>> 
>> Oh, actually just auto, realized this is a scalar.
> 
> It's ordered (alphabetically, with prefixed properties after unprefixed ones). Is there any concern about the ordering here? It should be preserved throughout, I think?

That’s great.

Might possibly rename computedPropertyIDs to express it’s ordering some day. Sounds more like a set than the "order properties should appear in web API".

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
>>> +    m_activeComputedPropertyIDs = WTFMove(active);
>> 
>> Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.
> 
> Perhaps a silly question: what's the easiest way to stack-allocate something to build it before moving it into a FixedVector?

The idiom I would have used would be an array, since these are scalars: either std::array or just a plain old C array depending on whether std::array helped make any of the code more elegant. That does require keeping count of the size in a separate variable. If we find that idiom too ugly we could create a tiny class template to make it more elegant, local to this file, or eventually moved to WTF if it’s a recurring pattern.

Could also use a Vector with inline capacity: that would still use only stack memory since we’d never get past the inline capacity, but would compile to a bit more code than we might want, with some extra indirection and branches deciding not to do heap allocation. So I’d use the array.

Given its name, it’s kind of non-obvious that FixedVector always implicitly uses the heap to optimize moving; might want to ask Yusuke about that at some point.

[Darin Adler]

Comment on attachment 456964 [details]
Patch

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

>>> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
>>> +    m_activeComputedPropertyIDs = WTFMove(active);
>> 
>> Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.
> 
> I couldn't figure out where I ought to put this; any suggestions? I duplicated it mostly to reach a point where I could get tests passing!

I typically choose mechanically rather than thinking too deeply. One possibility is to put it in the same header as the isCSSPropertyExposed function. Or it can go into ComputedStylePropertyMapReadOnly.h or CSSComputedStyleDeclaration.h. I think the function we want is:

    Vector<CSSPropertyID> activeComputedCSSPropertyIDs(Element&);

[Sam Sneddon [:gsnedders]]

Created attachment 459330 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 459336 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 459550 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 459624 [details]
Patch

[Sam Sneddon [:gsnedders]]

reviewers: plz re-review; might need to fix up one or two of the platform-specific expectation files, but don’t foresee any other changes now

[Tim Nguyen (:ntim)]

Comment on attachment 459624 [details]
Patch

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

> Source/WebCore/css/parser/CSSSelectorParser.cpp:652
> +            if (!RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled() && selector->pseudoClassType() == CSSSelector::PseudoClassHasAttachment)

I'm pretty sure we don't want people to use this singleton anymore.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:50
> +    auto settings = &element.document().settings();
> +    m_activeComputedPropertyIDs = getExposedCSSProperties(settings);

nit: could be one line.

> LayoutTests/imported/w3c/web-platform-tests/css/css-conditional/js/CSS-supports-CSSStyleDeclaration-expected.txt:895
> +FAIL font-display: _camel_cased_attribute v. CSS.supports assert_equals: expected true but got false
> +FAIL font-display: _dashed_attribute v. CSS.supports assert_equals: expected true but got false

Sorry if I missed this from earlier comments, but why is this now a failure?

> LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-cssfontrule.tentative.html:1
> +<!doctype html>

I don't particularly mind, but landing these new WPT in a different patch might make this easier to review/understand for others, and they also wouldn't get reverted with the full patch in case of regression.

> LayoutTests/platform/ios-wk2/editing/pasteboard/emacs-ctrl-k-y-001-expected.txt:16
> +          RenderBR {BR} at (65,1) size 1x28

Why is this new test result happening?

[Sam Sneddon [:gsnedders]]

Comment on attachment 459624 [details]
Patch

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

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:652
>> +            if (!RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled() && selector->pseudoClassType() == CSSSelector::PseudoClassHasAttachment)
> 
> I'm pretty sure we don't want people to use this singleton anymore.

We don't want people to use runtime features in general; that doesn't mean migrating everything away from it instantaneously. In this case, all that's happened is it no longer gets read into CSSParserContext for this single usage.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-conditional/js/CSS-supports-CSSStyleDeclaration-expected.txt:895
>> +FAIL font-display: _dashed_attribute v. CSS.supports assert_equals: expected true but got false
> 
> Sorry if I missed this from earlier comments, but why is this now a failure?

font-display is only a descriptor, and we still expose it on CSSStyleDeclaration, because fixing this up per CSS WG resolution (i.e., having a Firefox-like split of the actual properties onto another interface, what Firefox/DOM Style L2 calls CSS2Properties, but also adding a CSSFontRuleDeclaration with font-display) is a much larger change; this patch however stops it from showing up via CSS.supports (mostly because it was the only reasonable way to stop asserts from being easily hit from there!).

>> LayoutTests/platform/ios-wk2/editing/pasteboard/emacs-ctrl-k-y-001-expected.txt:16
>> +          RenderBR {BR} at (65,1) size 1x28
> 
> Why is this new test result happening?

Oh, this is unneeded because it's marked [ Failure ].

[Sam Sneddon [:gsnedders]]

Created attachment 459729 [details]
Patch

[Tim Nguyen (:ntim)]

I think it would be helpful if you started some perf runs to see if there are any regressions. I don't expect any, but it's not unlikely for a change like this.

[Sam Sneddon [:gsnedders]]

Created attachment 459759 [details]
Patch

[Sam Sneddon [:gsnedders]]

(In reply to Tim Nguyen (:ntim) from comment #43)
> I think it would be helpful if you started some perf runs to see if there
> are any regressions. I don't expect any, but it's not unlikely for a change
> like this.

I've done so internally, but on the whole I'd rather land this without waiting for that, and avoid having to spend yet more time having to rebase results (especially for platforms I don't have locally!).

[Sam Sneddon [:gsnedders]]

Created attachment 459784 [details]
Patch

[Darin Adler]

(In reply to Sam Sneddon [:gsnedders] from comment #45)
> (In reply to Tim Nguyen (:ntim) from comment #43)
> > I think it would be helpful if you started some perf runs to see if there
> > are any regressions. I don't expect any, but it's not unlikely for a change
> > like this.
> 
> I've done so internally, but on the whole I'd rather land this without
> waiting for that, and avoid having to spend yet more time having to rebase
> results (especially for platforms I don't have locally!).

I empathize, but I think this is backwards. We should get those test results *before* this is merged even if we risk the frustration of slowing this down further. Should take <24 hours with the A/B bots that are available to Apple engineers once you have a patch that builds on iOS and macOS; get help from someone who knows how if you haven’t done it before. I recently did it with a patch of mine for the first time (but sadly didn’t finish getting it to build on all platforms before I left on vacation, so likely won’t be able to land it until I get back to my computer at home).

[Sam Sneddon [:gsnedders]]

Created attachment 459818 [details]
Patch

[Sam Sneddon [:gsnedders]]

For the record: the perf runs showed no regression anywhere; the patch just uploaded will still fail fast/scrolling/ios/overflow-scrolling-touch-disabled-stacking.html on iOS (needs investigating, etc.), but everything else should be good now, and this at least uploads what I currently have before I go on vacation.

[Tim Nguyen (:ntim)]

(In reply to Sam Sneddon [:gsnedders] from comment #49)
> For the record: the perf runs showed no regression anywhere;
👍

> the patch just uploaded will still fail
> fast/scrolling/ios/overflow-scrolling-touch-disabled-stacking.html on iOS
> (needs investigating, etc.), but everything else should be good now, and
> this at least uploads what I currently have before I go on vacation.

I think it's the -webkit-overflow-scrolling CSS property which is causing this. Seems like it's enabled/exposed by default on iOS only. macOS does not have it enabled nor exposed (the build flag is off))

It seems gated behind shouldEnableLegacyOverflowScrollingTouch + the legacyOverflowScrollingTouchEnabled setting. Maybe it can be made internal only? or at least gated behind the setting?

https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-overflow-scrolling#browser_compatibility says it's pretty much useless at this point.

[Simon Fraser (smfr)]

-webkit-overflow-scrolling:touch still has the side-effect of making a scroller be a stacking context.

[Tim Nguyen (:ntim)]

Sam, sorry for the churn, but can you rebase on top of https://github.com/WebKit/WebKit/pull/993 ?

[Antti Koivisto]

Comment on attachment 459818 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.h:149
> +    FixedVector<CSSPropertyID> m_activeComputedPropertyIDs;

This is a vector containing every active CSSPropertyID so ~500 entries and 2 bytes per entry so ~1KB. This is being constructed from scratch for every style declaration created (which there can unbounded number). This seems way excessive in terms of both memory and processing time. The content of the vector is dependent on settings only so this structure should be per-document, not per declaration.

[Sam Sneddon [:gsnedders]]

Created attachment 460557 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 460586 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 460612 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 460618 [details]
I should debug why git-webkit isn't working for me again, but in the meantime I think this addresses all the above feedback. So, uh, r?.

[Sam Sneddon [:gsnedders]]

Created attachment 460619 [details]
Patch

[Darin Adler]

Looked at the iOS-WK2 EWS failure and it’s this line, missing from imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-actual.txt:

    PASS -webkit-tap-highlight-color

If this line being missing is an improvement and intentional, then I think we need to update LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt in a new version of the patch.

[Antti Koivisto]

Comment on attachment 460619 [details]
Patch

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

nice!

> Source/WebCore/css/CSSComputedStyleDeclaration.h:144
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const;

WebKit coding style does not use get* for getters (examples to the contrary are DOM API implementations or old leftovers). "Exposed" could  be a better name for this concept than "active computed", you use it elsewhere and in the prose too. So exposedCSSPropertyIDs() perhaps?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:355
> +bool PropertySetCSSStyleDeclaration::isCSSPropertyExposed(CSSPropertyID propertyID) const

or isExposedCSSProperty, similar to existing isInternalCSSProperty/isEnabledCSSProperty etc.

> Source/WebCore/dom/Document.cpp:648
> +    {
> +        std::array<CSSPropertyID, numComputedPropertyIDs> exposed;
> +        auto last = std::copy_if(std::begin(computedPropertyIDs), std::end(computedPropertyIDs),
> +            exposed.begin(), [&](CSSPropertyID x) {
> +                return isCSSPropertyExposed(x, m_settings.ptr());
> +            });
> +
> +        FixedVector<CSSPropertyID> active(std::distance(exposed.begin(), last));
> +        std::copy(exposed.begin(), last, active.begin());
> +        m_activeComputedPropertyIDs = active;
> +    }

This work is not needed unless the page actually uses computed style APIs. Initialization could by done lazily in getActiveComputedPropertyIDs (though it is not that expensive either).

> Source/WebCore/dom/Document.h:1604
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const { return m_activeComputedPropertyIDs; }

Similarly exposedCSSPropertyIDs (including "CSS" since the document scope doesn't imply it).

[Darin Adler]

More efficient to write WTFMove(active) to move the newly created vector in rather than copying it.

[Oriol Brufau]

Comment on attachment 460619 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:-1511
> -            if (!shorthandPropertyAppeared[borderProperty - firstCSSProperty] && isEnabledCSSProperty(borderProperty)) {

Why remove this? At some point, border-block was not exposed even though its longhands were. So the check was needed to avoid serializing with the shorthand.
I think the check should just be replaced with isCSSPropertyExposed.

> Source/WebCore/css/StyleProperties.cpp:-1771
> -        if (shorthandPropertyID && isEnabledCSSProperty(shorthandPropertyID)) {

Ditto, e.g. top,right,bottom,left have existed for a long time, but the inset shorthand is recent and for a while it was not exposed.

> Source/WebCore/css/makeprop.pl:569
> +    return isEnabledCSSProperty(id) && isCSSPropertyEnabledBySettings(id, settings) && !isInternalCSSProperty(id);

While you are at it refactoring all of this, I guess you can remove isEnabledCSSProperty since now there is nothing using runtime-flag.

[Sam Sneddon [:gsnedders]]

Created attachment 460751 [details]
Patch

[Sam Sneddon [:gsnedders]]

Comment on attachment 460619 [details]
Patch

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

See comments below for those I haven't addressed (and some detail when I have!):

>> Source/WebCore/css/CSSComputedStyleDeclaration.h:144
>> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const;
> 
> WebKit coding style does not use get* for getters (examples to the contrary are DOM API implementations or old leftovers). "Exposed" could  be a better name for this concept than "active computed", you use it elsewhere and in the prose too. So exposedCSSPropertyIDs() perhaps?

The computed properties are a subset of all the properties, and the exposed properties are a different subset of all the properties, so… changed this to exposedComputedCSSPropertyIDs.

>> Source/WebCore/css/StyleProperties.cpp:-1511
>> -            if (!shorthandPropertyAppeared[borderProperty - firstCSSProperty] && isEnabledCSSProperty(borderProperty)) {
> 
> Why remove this? At some point, border-block was not exposed even though its longhands were. So the check was needed to avoid serializing with the shorthand.
> I think the check should just be replaced with isCSSPropertyExposed.

Mostly because it's been a no-op for a while and nobody noticed, and it seems unlikely to be relevant again in future.

>> Source/WebCore/css/StyleProperties.cpp:-1771
>> -        if (shorthandPropertyID && isEnabledCSSProperty(shorthandPropertyID)) {
> 
> Ditto, e.g. top,right,bottom,left have existed for a long time, but the inset shorthand is recent and for a while it was not exposed.

Ah, yes, this probably makes more sense to preserve, though there isn't currently any reference to the CSSParserContext here. This also isn't a regression from this patch, given isEnabledCSSProperty has effectively been a no-op, so I suggest we deal with this as a follow-up?

>> Source/WebCore/css/makeprop.pl:569
>> +    return isEnabledCSSProperty(id) && isCSSPropertyEnabledBySettings(id, settings) && !isInternalCSSProperty(id);
> 
> While you are at it refactoring all of this, I guess you can remove isEnabledCSSProperty since now there is nothing using runtime-flag.

I deliberately didn't, because that probably should be a larger refactor of the Perl script.

[Tim Nguyen (:ntim)]

Created attachment 460768 [details]
Fixed iOS and mac-wk1 failures

Not sure why the results for mac-wk1 are different from mac-wk2 for all-prop-initial-xml.html:

-apple-color-filter is exposed only on mac-wk1
scroll-behavior is exposed only on mac-wk2


Looking at the preferences, I don't really see a good reason why this is the case.

[Sam Sneddon [:gsnedders]]

(In reply to Tim Nguyen (:ntim) from comment #65)
> Created attachment 460768 [details]
> Fixed iOS and mac-wk1 failures
> 
> Not sure why the results for mac-wk1 are different from mac-wk2 for
> all-prop-initial-xml.html:
> 
> -apple-color-filter is exposed only on mac-wk1

DRT enables it: https://searchfox.org/wubkat/rev/d7903e73fd92e1e091f668cf6658625471ad9a4d/Tools/DumpRenderTree/TestOptions.cpp#71

> scroll-behavior is exposed only on mac-wk2

DRT disables it: https://searchfox.org/wubkat/rev/d7903e73fd92e1e091f668cf6658625471ad9a4d/Tools/DumpRenderTree/TestOptions.cpp#110

> Looking at the preferences, I don't really see a good reason why this is the
> case.

[Sam Sneddon [:gsnedders]]

Created attachment 460823 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 461127 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 461128 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 461129 [details]
Patch

[EWS]

Committed 252720@main (d70cd13f2814): <https://commits.webkit.org/252720@main>

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