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.

Sign up for GitHub

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

Jump to bottom

Add block-step-size to CSS parser. #10176

Merged
merged 1 commit into from Feb 21, 2023
Merged

Add block-step-size to CSS parser. #10176

merged 1 commit into from Feb 21, 2023

Conversation

sammygill
Copy link
Contributor

@sammygill sammygill commented Feb 15, 2023 •
edited by webkit-early-warning-system

84095e2

Add block-step-size to CSS parser.
https://bugs.webkit.org/show_bug.cgi?id=252205
rdar://105421486

Reviewed by Tim Nguyen.

Grammar for property: none | <length [0,∞]>

We store the value within a std::optional<Length> within
StyleRareNonInheritedData. If we get an invalid value or "none," this
field will not have a value inside of it. Otherwise, we will store the
specified length as the value.

It seems like it is currently an open discussion as to whether it should
be animatable, so we mark it as a property that cannot be animated. If
the spec changes, then we will need to make the proper changes.

* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-computed-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-computed.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-invalid-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-invalid.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-valid-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-valid.html: Added.
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/rendering/style/RenderStyle.h:
(WebCore::RenderStyle::initialBlockStepSize):
(WebCore::RenderStyle::blockStepSize const):
(WebCore::RenderStyle::setBlockStepSize):
* Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:
(WebCore::StyleRareNonInheritedData::StyleRareNonInheritedData):
(WebCore::StyleRareNonInheritedData::operator== const):
* Source/WebCore/rendering/style/StyleRareNonInheritedData.h:
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertBlockStepSize):

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

ca1c50b

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🧪 bindings loading 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 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

@sammygill sammygill self-assigned this Feb 15, 2023
@sammygill sammygill added the CSS Cascading Style Sheets implementation label Feb 15, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 59c3981)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🛠 gtk
✅ 🧪 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
✅ 🛠 watch-sim

nt1m
nt1m approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment •
edited

Choose a reason for hiding this comment

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

You need to add tests for animation support. You'll likely need to rebase some tests too. See: af2d999

Source/WebCore/rendering/style/StyleRareNonInheritedData.h
@@ -173,6 +173,8 @@ class StyleRareNonInheritedData : public RefCounted<StyleRareNonInheritedData> {

float zoom;

std::optional<Length> blockStepSize;
Copy link
Member

Choose a reason for hiding this comment

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

You need to compare in the equality operator as well and add it to the copy constructor.

https://searchfox.org/wubkat/source/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp#161
https://searchfox.org/wubkat/rev/6312ca8a662145d355274780bbf68b6ce735d8e8/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp#283

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I got everything but let me know if I missed something

nt1m
nt1m reviewed Feb 16, 2023
View reviewed changes
LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-invalid.html Outdated
Comment on lines 4 to 5
<title>CSS Rhythm: block-step-size invalid values</title>
<title>CSS Rhythm: block-step-size valid values</title>
Copy link
Member

Choose a reason for hiding this comment

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

nit: double title

Suggested change
<title>CSS Rhythm: block-step-size invalid values</title>
<title>CSS Rhythm: block-step-size valid values</title>
<title>CSS Rhythm: block-step-size invalid values</title>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

nt1m
nt1m reviewed Feb 16, 2023
View reviewed changes
Source/WebCore/style/StyleBuilderConverter.h Outdated
@@ -1839,5 +1841,12 @@ inline TextAutospace BuilderConverter::convertTextAutospace(BuilderState&, const
return { };
}

inline std::optional<Length> BuilderConverter::convertBlockStepSize(BuilderState& builderState, const CSSValue& value)
{
if (auto primitiveValue = downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (auto primitiveValue = downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone)
if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

nt1m
nt1m reviewed Feb 16, 2023
View reviewed changes
Source/WebCore/animation/CSSPropertyAnimation.cpp
@@ -3662,6 +3662,7 @@ CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap()
case CSSPropertyAnimationPlayState:
case CSSPropertyAnimationTimingFunction:
case CSSPropertyAppearance:
case CSSPropertyBlockStepSize:
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, the spec doesn't define whether the property is animatable. Can you file an issue on w3c/csswg-drafts?

Feel free to ignore my comment about animation tests for now.

Copy link
Member

Choose a reason for hiding this comment

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

https://drafts.csswg.org/css-rhythm/#block-step-size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

w3c/csswg-drafts#8459

@sammygill sammygill force-pushed the eng/rhythmic-sizing-Add-block-step-size-to-CSS-parser branch from 59c3981 to 94751c6 Compare February 16, 2023 17:32
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 94751c6)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 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
✅ 🛠 watch-sim

nt1m
nt1m reviewed Feb 16, 2023
View reviewed changes
...utTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-computed.html Outdated
Comment on lines 4 to 5
<title>CSS Rhythm: block-step-size computed values</title>
<title>CSS Rhythm: block-step-size valid values</title>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<title>CSS Rhythm: block-step-size computed values</title>
<title>CSS Rhythm: block-step-size valid values</title>
<title>CSS Rhythm: block-step-size computed values</title>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to land, can you update the WPT PR too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, done!

@sammygill sammygill force-pushed the eng/rhythmic-sizing-Add-block-step-size-to-CSS-parser branch from 94751c6 to ca1c50b Compare February 20, 2023 19:37
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash ca1c50b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🧪 bindings loading 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 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

@sammygill sammygill added the merge-queue Applied to send a pull request to merge-queue label Feb 20, 2023
@sammygill
Add block-step-size to CSS parser.
84095e2 
https://bugs.webkit.org/show_bug.cgi?id=252205
rdar://105421486

Reviewed by Tim Nguyen.

Grammar for property: none | <length [0,∞]>

We store the value within a std::optional<Length> within
StyleRareNonInheritedData. If we get an invalid value or "none," this
field will not have a value inside of it. Otherwise, we will store the
specified length as the value.

It seems like it is currently an open discussion as to whether it should
be animatable, so we mark it as a property that cannot be animated. If
the spec changes, then we will need to make the proper changes.

* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-computed-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-computed.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-invalid-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-invalid.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-valid-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-rhythm/parsing/block-step-size-valid.html: Added.
* LayoutTests/platform/gtk/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/ipad/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/platform/wpe/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/rendering/style/RenderStyle.h:
(WebCore::RenderStyle::initialBlockStepSize):
(WebCore::RenderStyle::blockStepSize const):
(WebCore::RenderStyle::setBlockStepSize):
* Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:
(WebCore::StyleRareNonInheritedData::StyleRareNonInheritedData):
(WebCore::StyleRareNonInheritedData::operator== const):
* Source/WebCore/rendering/style/StyleRareNonInheritedData.h:
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertBlockStepSize):

Canonical link: https://commits.webkit.org/260574@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/rhythmic-sizing-Add-block-step-size-to-CSS-parser branch from ca1c50b to 84095e2 Compare February 21, 2023 00:06
@webkit-commit-queue
Copy link
Collaborator

Committed 260574@main (84095e2): https://commits.webkit.org/260574@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 21, 2023
@webkit-early-warning-system webkit-early-warning-system merged commit 84095e2 into WebKit:main Feb 21, 2023
@sammygill sammygill deleted the eng/rhythmic-sizing-Add-block-step-size-to-CSS-parser branch April 19, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nt1m nt1m nt1m approved these changes

@graouts graouts Awaiting requested review from graouts graouts is a code owner

@anttijk anttijk Awaiting requested review from anttijk

Assignees

@sammygill sammygill

Labels
CSS Cascading Style Sheets implementation
Projects
None yet
Milestone
No milestone
4 participants
@sammygill @webkit-early-warning-system @webkit-commit-queue @nt1m