Bug 223141 – Runtime-disabled CSS features still still appear enabled via two-arg CSS.supports()

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

[Sam Sneddon [:gsnedders]]

bug 222280 fixed:

> CSS.supports("overscroll-behavior: inherit")

< true

however, even after that:

> CSS.supports("overscroll-behavior: inherit")

< false

> CSS.supports("overscroll-behavior", "inherit")

< true

[Radar WebKit Bug Importer]

<rdar://problem/75377891>

[Simon Fraser (smfr)]

There's a different entry point for the comma separated list version: DOMCSSNamespace::supports(Document& document, const String& property, const String& value)

[Simon Fraser (smfr)]

Created attachment 423362 [details]
Patch

[Antti Koivisto]

Comment on attachment 423362 [details]
Patch

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

> Source/WebCore/css/DOMCSSNamespace.cpp:67
>  
> +    CSSParserContext parserContext(document);
> +    if (parserContext.isPropertyRuntimeDisabled(propertyID))
> +        propertyID = CSSPropertyInvalid;
> +
>      if (propertyID == CSSPropertyInvalid)
>          return false;

You can just return false in the first branch

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/274520/webkit

[Darin Adler]

Comment on attachment 423362 [details]
Patch

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

Tiny clean-up suggestions on moved code.

> Source/WebCore/css/parser/CSSParserContext.cpp:161
> +#if ENABLE(TEXT_AUTOSIZING)
> +    case CSSPropertyWebkitTextSizeAdjust:
> +#if !PLATFORM(IOS_FAMILY)
> +        return !textAutosizingEnabled;
> +#endif
> +        return false;
> +#endif // ENABLE(TEXT_AUTOSIZING)

This can be written in a less strange way:

#if ENABLE(TEXT_AUTOSIZING) && !PLATFORM(IOS_FAMILY)
    case CSSPropertyWebkitTextSizeAdjust:
        return !textAutosizingEnabled;
#endif

No need for the nested #if and double return statements.

> Source/WebCore/css/parser/CSSParserContext.cpp:169
> +    return false;

This unreachable code should be removed.

[Darin Adler]

Comment on attachment 423362 [details]
Patch

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

Tiny clean-up suggestions on moved code.

> Source/WebCore/css/parser/CSSParserContext.cpp:161
> +#if ENABLE(TEXT_AUTOSIZING)
> +    case CSSPropertyWebkitTextSizeAdjust:
> +#if !PLATFORM(IOS_FAMILY)
> +        return !textAutosizingEnabled;
> +#endif
> +        return false;
> +#endif // ENABLE(TEXT_AUTOSIZING)

This can be written in a less strange way:

#if ENABLE(TEXT_AUTOSIZING) && !PLATFORM(IOS_FAMILY)
    case CSSPropertyWebkitTextSizeAdjust:
        return !textAutosizingEnabled;
#endif

No need for the nested #if and double return statements.

> Source/WebCore/css/parser/CSSParserContext.cpp:169
> +    return false;

This unreachable code should be removed.

[Simon Fraser (smfr)]

Followups in https://trac.webkit.org/changeset/274725/webkit and https://trac.webkit.org/changeset/274756/webkit

[Karl Dubost]

Relevant WPT test
https://wpt.fyi/results/css/css-conditional/js/CSS-supports-CSSStyleDeclaration.html

[Simon Fraser (smfr)]

Is this bug only about `-epub` prefixed properties now?

[Karl Dubost]

This bug is marked as fixed. They are still a couple of failures, but I can open a separate bug. 

There are 
19 Fail including 9 about the prefix -epub-* which is converted as Epub by the test script.

Fail	-epub-caption-side: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-combine: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-emphasis: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-emphasis-color: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-emphasis-style: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-orientation: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-text-transform: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-word-break: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true
Fail	-epub-writing-mode: _webkit_cased_attribute must only exist for -webkit-	assert_false: expected false got true


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
Fail	inherits: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	initial-value: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	initial-value: _dashed_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	size: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	src: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	syntax: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	unicode-range: _camel_cased_attribute v. CSS.supports	assert_equals: expected true but got false
Fail	unicode-range: _dashed_attribute v. CSS.supports	assert_equals: expected true but got false