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

[css-text] text-spacing is very complicated #4246

Closed
litherum opened this issue Aug 27, 2019 · 17 comments
Closed

[css-text] text-spacing is very complicated #4246

litherum opened this issue Aug 27, 2019 · 17 comments
Labels
Closed Accepted by CSSWG Resolution css-text-4 i18n-clreq Chinese language enablement i18n-jlreq Japanese language enablement i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. Needs Testcase (WPT)

Comments

@litherum
Copy link
Contributor

text-spacing is getting very complicated now. By my count, the current grammar accepts 769 different combinations of values (770 if you count auto). It's probably worth an investigation to determine which of these combinations are actually useful.

@frivoal frivoal added i18n-clreq Chinese language enablement i18n-jlreq Japanese language enablement Needs i18n feedback labels Sep 10, 2019
@frivoal
Copy link
Collaborator

frivoal commented Sep 10, 2019

There probably are a number of unnecessary combinations in there indeed. We should get help from JLReq to figure which ones are essential.

cc: @r12a @macnmm @fantasai @MurakamiShinyu

@MurakamiShinyu
Copy link
Collaborator

I think that the text-spacing property is like a short hand property such as font-variant whose total number of combinations is very large and hard to reduce.

@litherum
Copy link
Contributor Author

litherum commented Sep 17, 2019

@dauwhe mentioned that Prince has something similar (prince-text-replace?). Perhaps he has some insight into how to better design text-spacing.

@dauwhe
Copy link
Contributor

dauwhe commented Sep 17, 2019

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.

prince-text-replace.pdf

@plehegar plehegar added i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. and removed Needs i18n feedback labels Dec 5, 2019
@kojiishi
Copy link
Contributor

kojiishi commented Dec 8, 2022

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 font-variant, having multiple longhands allows authors to change one of them without changing other values, such as enabling/disabling just ideograph-numeric oly for some text, or disabling no-compress only for a span.

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.

@macnmm
Copy link

macnmm commented Jan 5, 2023

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.

@litherum
Copy link
Contributor Author

litherum commented Jan 20, 2023

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).

@r12a
Copy link
Contributor

r12a commented Jan 20, 2023

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?

@frivoal
Copy link
Collaborator

frivoal commented Jan 25, 2023

@litherum I just did propose longhands for text-spacing as well in #8288 (comment) curious to see if you agree about the particular design proposed.

@fantasai
Copy link
Collaborator

fantasai commented Jan 26, 2023

OK, so @florian and I did some brainstorming, and here's a simplified version of text-spacing and possible longhand expansion that tries to address the comments in #8288 #7183 #4246 #8263 #318

Summary:

text-spacing: normal | none | auto | <'text-autospace'> || <'text-spacing-trim'>
  text-spacing-trim: auto | space-all |  trim-all | [ allow-end || space-first ]
  text-autospace: normal | auto | no-autospace |
                  [ ideograph-alpha || ideograph-numeric || punctuation ]
                  || [ insert | replace ]

Fullwidth Punctuation Trimming

text-spacing-trim (or text-spacing-full-width):
    auto | space-all |  trim-all | [ allow-end || space-first ]

This controls the spacing or trimming of fullwidth punctuations at the start, end, and adjacent positions. [ allow-end || space-first] are variations on trim-all. We don't think that there's a real need for these two values in combination with space-all, or with any other mix of the existing keywords; but if we wanted to allow the full combinations, we could insert them all here. The initial value is under debate in #2462 (and will likely be space-first).

Interscript and Punctuation Space Insertion

text-autospace:
    normal | auto | no-autospace |
    [ ideograph-alpha || ideograph-numeric || punctuation ]
    || [ insert | replace ]

The initial value would normal (currently defined as ideograph-alpha ideograph-numeric).

(punctuation is for languages like French, which want a narrow non-breaking space before ;:?! and an normal non-breaking space on the inside of « and ».)

To address one of @r12a’s requests in #7183 (split out as #8263), insert inserts spaces where there aren't any (as currently specced) while replace will do the same as insert, but also replace incorrect spaces (U+0020 and U+00A0) with correct spacing.

Compression Control

text-justify: <current-stuff> || no-compress

This moves no-compress into the text-justify property, which is the “how do I justify” control. It would not be reset by the text-spacing shorthand, which is probably better.

Shorthand
The shorthand for these would be:

text-spacing: normal | none | auto | <'text-autospace'> || <'text-spacing-trim'>

normal sets both to their initial values; none maps to no-autospace space-all (which is the “do nothing” value appropriate for e.g. monospace text, or to avoid compat issues with fiddly legacy content).


Excess Longhands
It might be reasonable for the two functions of text-autospace to cascade independently, so it could have further longhands (names TBD):

  • text-autospace-style: normal | no-autospace | auto | [cjk-alpha || cjk-num || punctuation ] with initial value normal
  • text-autospace-mode: insert | replace with initial value insert

text-spacing-trim could also have further long-hands as described in #8288 (comment), with a full explosion of possible combinations. However, these seem unnecessary (as most combinations aren't actually useful) and possibly a cascade footgun (if authors try and tweak something on top of an assumed base behavior using a longhand, without noticing that the other longhands have a cascaded behavior different from what they assumed would be there as the base behavior).

@r12a
Copy link
Contributor

r12a commented Jan 30, 2023

@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.

@SebastianZ
Copy link
Contributor

text-spacing: normal | none | auto | <'text-autospace'> || <'text-spacing-trim'>
  text-spacing-trim: auto | space-all |  trim-all | [ allow-end || space-first ]
  text-autospace: normal | auto | no-autospace |
                  [ ideograph-alpha || ideograph-numeric || punctuation ]
                  || [ insert | replace ]

I assume text-spacing: auto is equivalent to text-spacing-trim: auto and text-autospace: auto, right? If so, it doesn't have to be explicitly defined for the shorthand in the current syntax.

Sebastian

@r12a
Copy link
Contributor

r12a commented Jan 31, 2023

one thing i did notice is that there appears to be no way to set the width of the gap introduced by autospace.

I'm guessing that that may be because the French use case may require more than one width in the same paragraph?

insert inserts spaces where there aren't any

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".

webkit-early-warning-system pushed a commit to vitorroriz/WebKit that referenced this issue 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-early-warning-system pushed a commit to vitorroriz/WebKit that referenced this issue Feb 15, 2023
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
vitorroriz added a commit to vitorroriz/WebKit that referenced this issue Feb 15, 2023
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:
fantasai added a commit that referenced this issue Feb 21, 2023
* 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
@fantasai
Copy link
Collaborator

fantasai commented Feb 28, 2023

OK, we've edited in the refactoring described in #4246 (comment) as resolved by the CSSWG, along with the suggestion to rename trim-all to trim-auto per @MurakamiShinyu's #4246 (comment).

Follow-up issues filed:

Please file additional follow-up issues if anything else needs fixing. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution css-text-4 i18n-clreq Chinese language enablement i18n-jlreq Japanese language enablement i18n-tracker Group bringing to attention of Internationalization, or tracked by i18n but not needing response. Needs Testcase (WPT)
Projects
None yet
Development

No branches or pull requests