-
Notifications
You must be signed in to change notification settings - Fork 707
[css-text] text-spacing is very complicated #4246
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
Comments
There probably are a number of unnecessary combinations in there indeed. We should get help from JLReq to figure which ones are essential. |
I think that the |
@dauwhe mentioned that Prince has something similar ( |
Here's the PrinceXML property in action. It finds a period followed by a space, and replaces it with a period followed by an em-space (U+2003). PDF of the result attached. p.space {
prince-text-replace: ". " ".\2003";
} This has lots of problems, not least because you can't define under what context the replacements happen. |
How about considering splitting into multiple longhands? I recall that was the original plan of @fantasai when this property gets more matured? As suggested in #4246 (comment), similar to Doing so might also address #7183, and it looks like there's a mild consensus on in at TPAC. /cc @r12a For implementers, testing the parser for the 770 combinations of values without errors is not an easy task. Splitting to multiple longhands and using the existing already-tested mechnism for shothand helps. |
The JIS standard defines a set of character classes and their relationships in terms of a spacing range and compression/expansion priority, but there are problems with how these classes are defined (not differentiating between proportional/half-width/full-width for example) as well as no consideration of mixed script text. We need a modular approach that defines the character classes, the spacing ranges (with priority) of each class pairing, according to the needs of the UA and how fancy it wants to be when supporting complex layout. A reasonable default behavior could also be proposed as part of the jlreq, I would hope, so the spec could then be about a sparse override of the default. |
Agenda+ to discuss the possibility of splitting up the property into longhands (just like how we did with text-decoration-skip) text-spacing is something we’d like to start implementing soon (weeks not months). |
Note that, if nothing else, i'd very much like to split out autospace from text-spacing. See #7183. Can we please bring the discussion in that issue to the agenda along with this issue? |
@litherum I just did propose longhands for |
OK, so @florian and I did some brainstorming, and here's a simplified version of Summary:
Fullwidth Punctuation Trimming
This controls the spacing or trimming of fullwidth punctuations at the start, end, and adjacent positions. Interscript and Punctuation Space Insertion
The initial value would ( To address one of @r12a’s requests in #7183 (split out as #8263), Compression Control
This moves Shorthand
Excess Longhands
|
@fantasai i think this is a step in the right direction. I'm still trying to find the time to finish drilling into the detail fully, but one thing i did notice is that there appears to be no way to set the width of the gap introduced by autospace. That turned out to be something that content authors had different opinions about, or may use different widths in different contexts. |
I assume Sebastian |
I'm guessing that that may be because the French use case may require more than one width in the same paragraph?
I suggest using the word 'gap' for what is opened up by autospace, rather than 'space'. That will probably avoid a certain amount of confusion. So that quoted text would read "inserts a gap where there wasn't any". |
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
https://bugs.webkit.org/show_bug.cgi?id=252119 rdar://105340814 Reviewed by Myles C. Maxfield. text-spacing longhand: text-autospace parser for auto and no-autospace Reference: w3c/csswg-drafts#4246 (comment) Implement a parser for text-autospace (text-spacing longhand) for none and auto-space values. We can then iterate from here add remaining values. Syntax: text-autospace: auto | no-autospace * 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/mac-wk1/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property-002-expected.txt: * LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property-002-expected.txt: * LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property-002-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/CSSValueKeywords.in: * Source/WebCore/css/ComputedStyleExtractor.cpp: (WebCore::textAutospaceFromStyle): (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): * Source/WebCore/css/parser/CSSPropertyParser.cpp: (WebCore::initialValueForLonghand): * Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp: (WebCore::CSSPropertyParserHelpers::consumeTextAutospace): * Source/WebCore/css/parser/CSSPropertyParserHelpers.h: * Source/WebCore/platform/text/TextSpacing.h: (WebCore::TextAutospace::isAuto const): (WebCore::TextAutospace::isNoAutospace const): (WebCore::TextAutospace::operator== const): (WebCore::operator<<): * Source/WebCore/rendering/style/RenderStyle.cpp: (WebCore::RenderStyle::textAutospace const): * Source/WebCore/rendering/style/RenderStyle.h: (WebCore::RenderStyle::setTextAutospace): (WebCore::RenderStyle::initialTextAutospace): * 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::convertTextAutospace): Canonical link: https://commits.webkit.org/260306@main
https://bugs.webkit.org/show_bug.cgi?id=252068 rdar://105094870 Reviewed by NOBODY (OOPS!). Reference: w3c/csswg-drafts#4246 (comment) Implement a parser with longhands support for: text-spacing: auto | none | <text-autospace> || <text-spacing-trim> text-autospace: auto | no-autospace text-spacing-trim: auto | space-all We can then iterate from here adding support for 'normal' in the shorthand and for the remaining values in the longhands. For the text-spacing shorthand, apart from appearing as a single token, 'auto' can also appear as belonging to one of the longhands in any order. In case we parse an 'auto' value and the token range is not exhauted, there is a certain ambiguity if it belongs to the first or second longhand, since they can come in any order. Becuse all the other values of the longhands are unique, we can peek into the next token to solve this ambiguity. For the common case, where 'auto' doesn't appear as the first token, we try to re-use the parsing functions for the longhands. For that, we need to divide the range into 2 subranges, one for each longhand. We use again our knowledge of unique values for a longhand to identify such subranges. * LayoutTests/TestExpectations: * Source/WebCore/animation/CSSPropertyAnimation.cpp: (WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap): * Source/WebCore/css/CSSProperties.json: * Source/WebCore/css/ComputedStyleExtractor.cpp: (WebCore::textSpacingFromStyle): (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): * Source/WebCore/css/ShorthandSerializer.cpp: (WebCore::ShorthandSerializer::serialize): (WebCore::ShorthandSerializer::serializeTextSpacing const): * Source/WebCore/css/parser/CSSPropertyParser.cpp: (WebCore::CSSPropertyParser::consumeTextSpacing): (WebCore::CSSPropertyParser::parseShorthand): * Source/WebCore/css/parser/CSSPropertyParser.h: * Source/WebCore/css/parser/CSSPropertyParserHelpers.h: (WebCore::CSSPropertyParserHelpers::isValueIDUniqueToTextSpacingTrimLonghand): (WebCore::CSSPropertyParserHelpers::isValueIDUniqueToTextAutospaceLonghand): * Source/WebCore/platform/text/TextSpacing.h: (WebCore::operator<<): * Source/WebCore/rendering/style/RenderStyle.h:
* Remove non-useful keyword combinations #4246 #8288 * Split into longhands #4246 #7183 #8288 * Ensure off values for each thing #8288 #6950 * Add insert|replace to allow replacing incorrect space characters #318 #8263 #7183 * Make space-first the initial value #2462 * Allow hanging-punctuation to hang leading ideographic spaces #2462 * Move no-compress to text-justify #7079 See https://lists.w3.org/Archives/Public/www-style/2023Feb/0002.html
OK, we've edited in the refactoring described in #4246 (comment) as resolved by the CSSWG, along with the suggestion to rename Follow-up issues filed:
Please file additional follow-up issues if anything else needs fixing. Thanks! |
text-spacing
is getting very complicated now. By my count, the current grammar accepts 769 different combinations of values (770 if you countauto
). It's probably worth an investigation to determine which of these combinations are actually useful.The text was updated successfully, but these errors were encountered: