Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@property] Fallback that does not match syntax should make var() invalid #8116

Merged
merged 1 commit into from Jan 1, 2023

Conversation

anttijk
Copy link
Contributor

@anttijk anttijk commented Jan 1, 2023

6dabb8f

[@property] Fallback that does not match syntax should make var() invalid
https://bugs.webkit.org/show_bug.cgi?id=249981
rdar://103799338

Reviewed by Sam Weinig.

The fallback value must match the syntax definition of the custom property being referenced,
otherwise the declaration is invalid at computed-value time.

https://drafts.css-houdini.org/css-properties-values-api/#fallbacks-in-var-references

* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/var-reference-registered-properties-expected.txt:
* Source/WebCore/css/CSSVariableReferenceValue.cpp:
(WebCore::CSSVariableReferenceValue::resolveVariableFallback const):

Check the syntax of the fallback.

(WebCore::CSSVariableReferenceValue::resolveVariableReference const):
(WebCore::CSSVariableReferenceValue::resolveTokenRange const):
(WebCore::CSSVariableReferenceValue::resolveVariableReferences const):
(WebCore::CSSVariableReferenceValue::resolveVariableFallback): Deleted.
(WebCore::CSSVariableReferenceValue::resolveVariableReference): Deleted.
(WebCore::CSSVariableReferenceValue::resolveTokenRange): Deleted.

Make these non-static (so parser context is available) and did some return value cleanup.

* Source/WebCore/css/CSSVariableReferenceValue.h:
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::CSSPropertyParser::isValidCustomPropertyValueForSyntax):
* Source/WebCore/css/parser/CSSPropertyParser.h:

Canonical link: https://commits.webkit.org/258373@main

9edf1da

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe ❌ πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
❌ πŸ§ͺ api-ios   πŸ§ͺ mac-wk1 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch   πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@anttijk anttijk self-assigned this Jan 1, 2023
@anttijk anttijk added the CSS Cascading Style Sheets implementation label Jan 1, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 1, 2023
@anttijk anttijk removed the merging-blocked Applied to prevent a change from being merged label Jan 1, 2023
return nullptr;

return CSSVariableData::create(resolvedTokens);
return CSSVariableData::create(*resolvedTokens);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unfortunate we have to copy here. Can we add a CSSVariableData::create() overload that takes an r-value and move the resolved tokens in? Or does the processing done in CSSVariableData's constructor make this a meaning less optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something to do separately. I did notice it and wondered about the same things you did.

@anttijk anttijk added the merge-queue Applied to send a pull request to merge-queue label Jan 1, 2023
…alid

https://bugs.webkit.org/show_bug.cgi?id=249981
rdar://103799338

Reviewed by Sam Weinig.

The fallback value must match the syntax definition of the custom property being referenced,
otherwise the declaration is invalid at computed-value time.

https://drafts.css-houdini.org/css-properties-values-api/#fallbacks-in-var-references

* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/var-reference-registered-properties-expected.txt:
* Source/WebCore/css/CSSVariableReferenceValue.cpp:
(WebCore::CSSVariableReferenceValue::resolveVariableFallback const):

Check the syntax of the fallback.

(WebCore::CSSVariableReferenceValue::resolveVariableReference const):
(WebCore::CSSVariableReferenceValue::resolveTokenRange const):
(WebCore::CSSVariableReferenceValue::resolveVariableReferences const):
(WebCore::CSSVariableReferenceValue::resolveVariableFallback): Deleted.
(WebCore::CSSVariableReferenceValue::resolveVariableReference): Deleted.
(WebCore::CSSVariableReferenceValue::resolveTokenRange): Deleted.

Make these non-static (so parser context is available) and did some return value cleanup.

* Source/WebCore/css/CSSVariableReferenceValue.h:
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::CSSPropertyParser::isValidCustomPropertyValueForSyntax):
* Source/WebCore/css/parser/CSSPropertyParser.h:

Canonical link: https://commits.webkit.org/258373@main
@webkit-commit-queue
Copy link
Collaborator

Committed 258373@main (6dabb8f): https://commits.webkit.org/258373@main

Reviewed commits have been landed. Closing PR #8116 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 6dabb8f into WebKit:main Jan 1, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 1, 2023
@anttijk anttijk deleted the prop-fallback branch January 3, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
5 participants