Bug 192800

Summary: Update CSS Properties and Values API to use new cycle fallback behaviour
Product: WebKit Reporter: Justin Michaud <justin_michaud>
Component: Layout and RenderingAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dino, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Justin Michaud 2018-12-17 21:26:55 PST
I re-imported the WPT tests for the CSS Properties and Values API, and it it seems that chromium developers changed the behaviour of fallbacks with cycles.

Before, if a cycle was detected, the registered variable acted like it was unset. This matches the spec (see the font-size example): <https://drafts.css-houdini.org/css-properties-values-api/#dependency-cycles-via-relative-units>.

Now, it acts like it is invalid. This seems to fit with the spirit of CSS variables more, although I could not find a spec bug documenting this change. I might not be searching correctly, although it would be nice if the WPT pr linked to it if it existed. I did find the Chromium PR to the WPT repo which changed the tests: <https://github.com/web-platform-tests/wpt/pull/14039>.
Comment 1 Justin Michaud 2018-12-17 21:30:03 PST
Created attachment 357532 [details]
Patch
Comment 2 Antti Koivisto 2018-12-18 00:17:38 PST
Comment on attachment 357532 [details]
Patch

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

> Source/WebCore/css/CSSVariableReferenceValue.cpp:89
> +    if ((!property || property->isInvalid()) && fallbackReturn) {
> +        result.appendVector(fallbackResult);
> +        return fallbackReturn;
> +    }
> +
> +    if (!property || property->isInvalid())
> +        return false;

More compactly:

if (!property || property->isInvalid()) {
    if (fallbackReturn)
        result.appendVector(fallbackResult);
    return fallbackReturn;
}

> LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/register-property-syntax-parsing-expected.txt:28
> -PASS syntax:'<length-percentage>', initialValue:'calc(-11px + 10.4%)' is valid 
> +FAIL syntax:'<length-percentage>', initialValue:'calc(-11px + 10.4%)' is valid The given initial value does not parse for the given syntax.

Why do some of these go from PASS to FAIL? You could mention it in the ChangeLog.
Comment 3 Justin Michaud 2018-12-18 10:10:36 PST
Created attachment 357578 [details]
Patch
Comment 4 Justin Michaud 2018-12-18 15:33:28 PST
Created attachment 357624 [details]
Patch
Comment 5 WebKit Commit Bot 2018-12-18 19:21:23 PST
Comment on attachment 357624 [details]
Patch

Clearing flags on attachment: 357624

Committed r239365: <https://trac.webkit.org/changeset/239365>
Comment 6 WebKit Commit Bot 2018-12-18 19:21:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-12-18 19:22:25 PST
<rdar://problem/46829637>