Bug 232868

Summary: Use isValidCustomIdentifier in consumeWillChange
Product: WebKit Reporter: Tim Nguyen (:ntim) <ntim>
Component: CSSAssignee: Tim Nguyen (:ntim) <ntim>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, koivisto, macpherson, menard, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/31556
Bug Depends on:    
Bug Blocks: 224816    
Attachments:
Description Flags
Patch
koivisto: review+
Patch
none
Patch none

Description Tim Nguyen (:ntim) 2021-11-08 22:50:03 PST 
This is what the spec says: https://drafts.csswg.org/css-will-change/#will-change

> The <custom-ident> production used here excludes the keywords will-change, none, all, auto, scroll-position, and contents, in addition to the keywords normally excluded from <custom-ident>.
Comment 1 Tim Nguyen (:ntim) 2021-11-08 22:55:08 PST 
Created attachment 443655 [details]
Patch
Comment 2 Antti Koivisto 2021-11-08 23:39:50 PST 
Comment on attachment 443655 [details]
Patch

Can you either include the relevant WPT merge with the patch or merge the WPT first? Landing behaviour changes without tests is not nice.
Comment 3 Tim Nguyen (:ntim) 2021-11-09 00:44:53 PST 
Created attachment 443657 [details]
Patch
Comment 4 EWS Watchlist 2021-11-09 00:45:55 PST 
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
Comment 5 Tim Nguyen (:ntim) 2021-11-09 01:02:40 PST 
Created attachment 443659 [details]
Patch
Comment 6 EWS 2021-11-09 01:31:08 PST 
Committed r285487 (244011@main): <https://commits.webkit.org/244011@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443659 [details].
Comment 7 Radar WebKit Bug Importer 2021-11-09 01:32:24 PST 
<rdar://problem/85193689>
Comment 8 Antti Koivisto 2021-11-09 02:16:19 PST 
Comment on attachment 443655 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:517
> +                if (!isValidCustomIdentifier(id))
> +                    return nullptr;
>                  // Append properties we don't recognize, but that are legal, as strings.
>                  values->append(consumeCustomIdent(range).releaseNonNull());

consumeCustomIdent should ASSERT(isValidCustomIdentifier) to catch all these.
Comment 9 Antti Koivisto 2021-11-09 02:34:39 PST 
Comment on attachment 443659 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:517
> +                if (!isValidCustomIdentifier(id))
> +                    return nullptr;
>                  // Append properties we don't recognize, but that are legal, as strings.
>                  values->append(consumeCustomIdent(range).releaseNonNull());

Actually since consumeCustomIdent already check isValidCustomIdentifier() a better fix here would be to just null chack consumeCustomIdent result