Skip to content

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Oct 12, 2024

43585a4

text-spacing: text-spacing-trim: implement trim-all for simple path
https://bugs.webkit.org/show_bug.cgi?id=281371
rdar://137795775

Reviewed by Brent Fulgham.

Implementing text-spacing: trim-all for the simple path (WidthIterator).

* Source/WTF/wtf/text/CharacterProperties.h:
(WTF::isFullwidthMiddleDotPunctuation):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeTextSpacingTrim): Deleted.
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:
* Source/WebCore/platform/graphics/WidthIterator.cpp:
(WebCore::WidthIterator::advanceInternal):
* Source/WebCore/platform/text/TextSpacing.cpp:
(WebCore::TextSpacingTrim::shouldTrimSpacing const):
(WebCore::TextSpacing::characterClass):
* Source/WebCore/platform/text/TextSpacing.h:
(WebCore::operator<<):
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertTextSpacingTrim):

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

ddb25f5

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
🛠 🧪 jsc 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv 🛠 mac-safer-cpp ✅ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@vitorroriz vitorroriz self-assigned this Oct 12, 2024
@vitorroriz vitorroriz added the Text For bugs in text layout and rendering, including international text support. label Oct 12, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 12, 2024
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #1451

❌ Found 1 new failure. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@vitorroriz vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Oct 12, 2024
@vitorroriz vitorroriz force-pushed the text-spacing-trim-all branch from 6817f64 to 247e5d4 Compare October 12, 2024 21:29
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 12, 2024
@vitorroriz vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Oct 13, 2024
@vitorroriz vitorroriz changed the title text-spacing: text-spacing-trim: text-spacing: text-spacing-trim: implement trim-all for simple path Oct 13, 2024
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

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

I had a few nits, but I don't need to review this again. Please do correct the naming of ...IdentifierKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we need the allow-end or space-first unused items any longer? Are they covered by the space-all and other grammars?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason all of this doesn't live in a FontCoreText.mm file? Then all of this could be done using Objective-C iterations, etc.

@vitorroriz
Copy link
Contributor Author

This patch is rebased on top of #35074. So I'm resolving Brent's review regarding the first patch there.

@vitorroriz vitorroriz force-pushed the text-spacing-trim-all branch from 247e5d4 to 696738a Compare October 15, 2024 20:20
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 15, 2024
@vitorroriz vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Oct 16, 2024
@vitorroriz vitorroriz force-pushed the text-spacing-trim-all branch from 696738a to ddb25f5 Compare October 16, 2024 18:40
@vitorroriz vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Oct 16, 2024
https://bugs.webkit.org/show_bug.cgi?id=281371
rdar://137795775

Reviewed by Brent Fulgham.

Implementing text-spacing: trim-all for the simple path (WidthIterator).

* Source/WTF/wtf/text/CharacterProperties.h:
(WTF::isFullwidthMiddleDotPunctuation):
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeTextSpacingTrim): Deleted.
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:
* Source/WebCore/platform/graphics/WidthIterator.cpp:
(WebCore::WidthIterator::advanceInternal):
* Source/WebCore/platform/text/TextSpacing.cpp:
(WebCore::TextSpacingTrim::shouldTrimSpacing const):
(WebCore::TextSpacing::characterClass):
* Source/WebCore/platform/text/TextSpacing.h:
(WebCore::operator<<):
* Source/WebCore/style/StyleBuilderConverter.h:
(WebCore::Style::BuilderConverter::convertTextSpacingTrim):

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

Committed 285287@main (43585a4): https://commits.webkit.org/285287@main

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

@webkit-commit-queue webkit-commit-queue merged commit 43585a4 into WebKit:main Oct 16, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Text For bugs in text layout and rendering, including international text support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants