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

Implement text-spacing-trim parser for auto and space-all #9988

Merged

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Feb 12, 2023

4cee70d

Implement text-spacing-trim parser for auto and space-all
https://bugs.webkit.org/show_bug.cgi?id=252124
rdar://105342875

Reference: w3c/csswg-drafts#4246 (comment)

Implement a partial parser for text-spacing-trim (text-spacing longhand)
for auto and space-all values. We can then iterate from here adding
the remaining values once we commit to handle them.

Syntax:
text-spacing-trim: auto | space-all

Reviewed by Myles C. Maxfield.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/property-list.js:
* 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/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/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Headers.cmake:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::textSpacingTrimFromStyle):
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::initialValueForLonghand):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeTextSpacingTrim):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:
* Source/WebCore/platform/text/TextSpacing.h: Added.
(WebCore::TextSpacingTrim::isAuto const):
(WebCore::TextSpacingTrim::isSpaceAll const):
(WebCore::TextSpacingTrim::operator== const):
* Source/WebCore/rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::textSpacingTrim const):
* Source/WebCore/rendering/style/RenderStyle.h:
(WebCore::RenderStyle::setTextSpacingTrim):
(WebCore::RenderStyle::initialTextSpacingTrim):
* Source/WebCore/rendering/style/RenderStyleConstants.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/RenderStyleConstants.h:
* Source/WebCore/rendering/style/StyleRareInheritedData.cpp:
(WebCore::StyleRareInheritedData::StyleRareInheritedData):
(WebCore::StyleRareInheritedData::operator== const):
* Source/WebCore/rendering/style/StyleRareInheritedData.h:
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertTextSpacingTrim):

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

22c8777

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
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2   πŸ›  jsc-armv7
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2   πŸ§ͺ jsc-armv7-tests
  πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ›  jsc-mips
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim   πŸ§ͺ jsc-mips-tests

@vitorroriz vitorroriz self-assigned this Feb 12, 2023
@vitorroriz vitorroriz added the CSS Cascading Style Sheets implementation label Feb 12, 2023
@vitorroriz vitorroriz changed the title Need a short description (OOPS!). Implement text-spacing-trim parser for auto and space-all Feb 12, 2023
@@ -3492,6 +3493,7 @@ CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap()
new DiscretePropertyWrapper<WhiteSpace>(CSSPropertyWhiteSpace, &RenderStyle::whiteSpace, &RenderStyle::setWhiteSpace),
new DiscretePropertyWrapper<WordBreak>(CSSPropertyWordBreak, &RenderStyle::wordBreak, &RenderStyle::setWordBreak),
new DiscretePropertyWrapper<OverflowAnchor>(CSSPropertyOverflowAnchor, &RenderStyle::overflowAnchor, &RenderStyle::setOverflowAnchor),
new DiscretePropertyWrapper<TextSpacingTrim>(CSSPropertyTextSpacingTrim, &RenderStyle::textSpacingTrim, &RenderStyle::setTextSpacingTrim),
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like there is testing coverage for this. Could you add the appropriate entry to web-animations/animation-model/animation-types/property-list.js? Should be something like this:

  'text-spacing-trim': {
    // https://drafts.csswg.org/link-to-spec
    types: [
      { type: 'discrete' , options: [ [ 'space-all', 'trim-all' ] ] }
    ]
  },

You can feel free to expand on that of course.

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, thanks.

"settings-flag": "cssTextSpacingEnabled",
"converter": "TextSpacingTrim",
"parser-function": "consumeTextSpacingTrim",
"parser-grammar-unused": "auto | space-all | trim-all | [ allow-end || space-first ]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space before trim-all.

Comment on lines 2676 to 2683
if (existingValue && existingValue->equals(value.get()))
return;
data.access().customProperties.access().setCustomPropertyValue(name, WTFMove(value));
if (existingValue && existingValue->equals(value.get()))
return;
data.access().customProperties.access().setCustomPropertyValue(name, WTFMove(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

That change looks wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this change looks wrong

@graouts
Copy link
Contributor

graouts commented Feb 13, 2023

You need to export those changes to the WPT repository. You can automate that using export-w3c-test-changes.

@vitorroriz
Copy link
Contributor Author

You need to export those changes to the WPT repository. You can automate that using export-w3c-test-changes.

We don't want to do that yet. The spec is in flux and we are implementing partially based on the CSSWG resolution not yet converted into the draft.

if (textSpacingTrim.isAuto())
return CSSPrimitiveValue::create(CSSValueAuto);

return CSSPrimitiveValue::create(CSSValueSpaceAll);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe ASSERT() here?


namespace WebCore {

WTF::TextStream& operator<<(WTF::TextStream& ts, const TextSpacingTrim& value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need a whole file just for this? Don't we have somewhere else that has a whole bunch of these?

Comment on lines 2676 to 2683
if (existingValue && existingValue->equals(value.get()))
return;
data.access().customProperties.access().setCustomPropertyValue(name, WTFMove(value));
if (existingValue && existingValue->equals(value.get()))
return;
data.access().customProperties.access().setCustomPropertyValue(name, WTFMove(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this change looks wrong

@vitorroriz vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Feb 15, 2023
https://bugs.webkit.org/show_bug.cgi?id=252124
rdar://105342875

Reference: w3c/csswg-drafts#4246 (comment)

Implement a partial parser for text-spacing-trim (text-spacing longhand)
for auto and space-all values. We can then iterate from here adding
the remaining values once we commit to handle them.

Syntax:
text-spacing-trim: auto | space-all

Reviewed by Myles C. Maxfield.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property-002-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/property-list.js:
* 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/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/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Headers.cmake:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/animation/CSSPropertyAnimation.cpp:
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::textSpacingTrimFromStyle):
(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::initialValueForLonghand):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeTextSpacingTrim):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:
* Source/WebCore/platform/text/TextSpacing.h: Added.
(WebCore::TextSpacingTrim::isAuto const):
(WebCore::TextSpacingTrim::isSpaceAll const):
(WebCore::TextSpacingTrim::operator== const):
* Source/WebCore/rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::textSpacingTrim const):
* Source/WebCore/rendering/style/RenderStyle.h:
(WebCore::RenderStyle::setTextSpacingTrim):
(WebCore::RenderStyle::initialTextSpacingTrim):
* Source/WebCore/rendering/style/RenderStyleConstants.cpp:
(WebCore::operator<<):
* Source/WebCore/rendering/style/RenderStyleConstants.h:
* Source/WebCore/rendering/style/StyleRareInheritedData.cpp:
(WebCore::StyleRareInheritedData::StyleRareInheritedData):
(WebCore::StyleRareInheritedData::operator== const):
* Source/WebCore/rendering/style/StyleRareInheritedData.h:
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertTextSpacingTrim):

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

Committed 260288@main (4cee70d): https://commits.webkit.org/260288@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 4cee70d into WebKit:main Feb 15, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 15, 2023
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