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-fonts-5] font-size-adjust: ic-height #8792

Closed
shivamidow opened this issue May 4, 2023 · 8 comments
Closed

[css-fonts-5] font-size-adjust: ic-height #8792

shivamidow opened this issue May 4, 2023 · 8 comments
Assignees

Comments

@shivamidow
Copy link
Member

shivamidow commented May 4, 2023

Related Spec: https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
Related Issues: #6288, #6384

Since #6288, the ic metric split into ic-width and ic-height. And the latest draft describes them as follows.

"ic-width"
Normalize the horizontal wide pitch of the font, using the advance width of “水” (CJK water ideograph, U+6C34) divided by the font size.
"ic-height"
Normalize the vertical wide pitch of the font, using the advance height of “水” (CJK water ideograph, U+6C34) divided by the font size.

While implementing these metrics in Blink and WebKit, it was questionable whether having these two metrics for the ideogram is worth it. Here is my rationale:

  1. Many fonts shape “水” in a square. Even if that is not a perfect square, the difference in height and width is small. So, I doubt if we have a practical use case that we should differentiate ic-height and ic-width for font-size-adjust.
  2. Many fonts often omit vertical metrics, so we cannot get the advance height of “水” in a reliable way. The draft does not clarify a fallback behavior for the absent case. Interestingly, I found Gecko uses an average character width for the fallback [1, 2], although using ic-width instead makes more sense to me. We can consider approximating the advance height from the rendering result, but that is not preferred because different rendering systems could incur another flaky difference in layout. In any case, the spec document needs to clarify it. Otherwise, the web designer would be hard to figure out why ic-height does not work as expected.

Proposed solution:
I propose removing ic-height from the spec as its merit of keeping ic-height seems low for now.

[1] https://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFonts.cpp#345
[2] https://searchfox.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#1058

@drott drott removed their assignment May 4, 2023
@jfkthame
Copy link
Contributor

jfkthame commented May 4, 2023

Interestingly, I found Gecko uses an average character width for the fallback [1, 2],

Note that when the fallback uses GetMetrics(nsFontMetrics::eVertical).aveCharWidth, this is actually a vertical advance (i.e. height), not width. The gfxFont::Metrics struct has legacy field names where a number of the fields have "width" in their names, but for vertical-mode metrics these are actually advance heights.

So the fallback used is the "average" vertical advance, which in turn is based on the font's ascent + descent, as there's no vertical-mode equivalent to the OS/2 table's xAvgCharWidth field.

I think this is better than falling back to ic-width because of the possibility of significantly non-square fonts. I know most common fonts will have square or near-square glyphs for 水 and other ideographs, but in a highly-stylised (e.g. condensed or expanded) font this might not be true. So Gecko's implementation is instead based on the assumption that in vertical mode, all characters will simply get the same fixed advance (height) if specific vertical metrics are not present.

@shivamidow
Copy link
Member Author

So the fallback used is the "average" vertical advance, which in turn is based on the font's ascent + descent, as there's no vertical-mode equivalent to the OS/2 table's xAvgCharWidth field.

Thanks for the explanation. If I understand correctly, the advance height differs slightly from the height (i.e., the sum of ascent and descent). The advance height indicates the next vertical pen position after a glyph so that it can include some marginal space as well as ascent and descent [1]. So, strictly speaking, the aveCharWidth can also cause a difference from what the spec states. using the *advance* height of “水”. Or else, is what the spec really wants to say using the height of “水”, not the advance height?

[1] https://freetype.org/freetype2/docs/glyphs/glyphs-3.html

I think this is better than falling back to ic-width because of the possibility of significantly non-square fonts. I know most common fonts will have square or near-square glyphs for 水 and other ideographs, but in a highly-stylised (e.g. condensed or expanded) font this might not be true. So Gecko's implementation is instead based on the assumption that in vertical mode, all characters will simply get the same fixed advance (height) if specific vertical metrics are not present.

O.K. If we keep ic-height, the spec at least needs to be updated, clarifying a fallback behavior. Otherwise, it would confuse web designers in the fallback situation.

So far, we have the following options.

  1. Remove ic-height from the spec
  2. Use the average vertical height as a fallback if “水” is absent as Gecko does.
  3. No adjustment if we cannot get a necessary metric from a font, as discussed in [css-sizing-3] web-compat/impl check for max-width percentages’ min-content compression #6348.
  4. Change the definition of ic-height to something else like ch-height. e.g., using the advance height of “水” -> using the height of “0”

Any other ideas?

@fantasai
Copy link
Collaborator

fantasai commented Jun 1, 2023

Another option would be to use, as a fallback, the distance between the ideographic top and ideographic bottom baselines (if they exist).

@jfkthame
Copy link
Contributor

jfkthame commented Jun 1, 2023

In theory, I guess that could be reasonable. In practice, I don't recall ever running across a font that lacked vertical-layout metrics, but did define the various ideographic baselines, so I'm not sure how useful it'd be as a fallback.

shivamidow added a commit to shivamidow/WebKit that referenced this issue Nov 20, 2023
https://bugs.webkit.org/show_bug.cgi?id=264064

Reviewed by NOBODY (OOPS!).

This change brings the ic-height metric support for font-size-adjust [1] into
ports using OpenType. As initializing glyphs' vertical data is expensive, we defer
it until the vertical data is requested. If a used font does not provide height
of the cjk water glyph, we use its width as a fallback [2] because the glyph usually
has the same width and height.

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792

Test: imported/w3c/web-platform-tests/css/css-fonts/font-size-adjust-ic-height.html

* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/font-size-adjust-ic-height-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/font-size-adjust-ic-height-ref.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/font-size-adjust-ic-height.html: Added.
* Source/WebCore/platform/graphics/Font.cpp:
(WebCore::m_shouldNotBeUsedForArabic):
(WebCore::Font::platformGlyphInit):
(WebCore::Font::platformGlyphVerticalDataInit):
(WebCore::Font::heightForGlyph const):
* Source/WebCore/platform/graphics/Font.h:
(WebCore::Font::widthForGlyph const):
* Source/WebCore/platform/graphics/FontMetrics.h:
(WebCore::FontMetrics::ideogramHeight const):
(WebCore::FontMetrics::reset):
* Source/WebCore/platform/graphics/FontSizeAdjust.h:
(WebCore::FontSizeAdjust::resolve const):
* Source/WebCore/rendering/RenderFileUploadControl.cpp:
(WebCore::RenderFileUploadControl::paintControl):
* Source/WebCore/style/StyleFontSizeFunctions.cpp:
(WebCore::Style::adjustedFontSize):
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 12, 2023
This CL adds the ic-height metric support to font-size-adjust [1]. The
ic-height metric lets developers adjust font size relative to the height
of the CJK water glyph. A note-worthy is the fallback case where the
used font does not provide vertical font info. Such vertical font info
is often missing, but the current specification does not describe the
fallback behavior. As a temporary measure, we use the advance width of
the CJK water glyph instead until the standard fallback behavior is
established [2] if we cannot get its advance height from the font. That
would be a sensible approximation since the CJK water glyph has the same
width and height in most fonts.

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792

Test:
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 13, 2023
This CL adds the ic-height metric support to font-size-adjust [1]. The
ic-height metric lets developers adjust font size relative to the height
of the CJK water glyph. A note-worthy is the fallback case where the
used font does not provide vertical font info. Such vertical font info
is often missing, but the current specification does not describe the
fallback behavior. As a temporary measure, we use the advance width of
the CJK water glyph instead until the standard fallback behavior is
established [2] if we cannot get its advance height from the font. That
would be a sensible approximation since the CJK water glyph has the same
width and height in most fonts.

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792

Test:
animations/responsive/interpolation/font-size-adjust-responsive.html
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
@shivamidow
Copy link
Member Author

I hope we can resolve this issue soon. This is a blocker for me to implement ic-height in Blink and WebKit. While I still prefer to remove ic-height from the spec, I am open to other options.

I noticed that the Noto Sans CJK JP font in the WPT repo [1] supports vertical-layout information. Here are some numbers I got from the font with a font-size of 200px:

  • Advance Height of “水”: 200px
  • Advance Width of “水”: 200px
  • Height (Ascent + Descent): 290px
  • Ascent: 232px
  • Average Character Width: 99.599998px

Given this information, I think the advance width of "水" is the most sensible alternative for 'ic-height' if we decide to keep it in the spec. If that doesn't work, my last preference is a disregard (i.e., no effect) as the fallback behavior.

To sum up, my preference goes in the following order:

  1. Removing ic-height from the spec
  2. Using advance width of “水” as the fallback
  3. Disregarding ic-height with no effect as the fallback.

[1] /fonts/noto/cjk/NotoSansCJKjp-Regular-subset-chws.otf

@jfkthame
Copy link
Contributor

What do Blink and WebKit use as the vertical advance of "水" if they're rendering writing-mode: vertical-rl content but the font being used has no vertical-layout metrics? I think I'd expect ic-height to match whatever fallback is used there.

@shivamidow
Copy link
Member Author

What do Blink and WebKit use as the vertical advance of "水" if they're rendering writing-mode: vertical-rl content but the font being used has no vertical-layout metrics? I think I'd expect ic-height to match whatever fallback is used there.

Blink and WebKit use the font height (i.e., Ascent + Descent) as the fallback.

I do not object to having the font height as the fallback for ic-height as long as the spec document clarifies it. But I want to note two concerns here.

  1. As we can see from the numbers above, a non-negligible difference exists between the actual advance height of “水” and the ascent + descent values.
  2. In writing-mode: vertical-rl, the ascent + descent may not represent the CJK font height. While ic-height is absolute regardless of the text orientation, the font height (i.e., ascent + descent) is not. Please refer to case [B] at [css-fonts-5] Do ic-width and ic-height change depending on text orientations? #9599. In [B], the ascent + descent represents a horizontal measure rather than a vertical one, so it could make users confused in deciding the scaling level by font-size-adjust.

@shivamidow
Copy link
Member Author

Originally, I intended to remove ic-height, but our conversation so far suggests retaining it and using a fallback if its necessary metric can't be obtained from a font. This contradicts the majority opinion in #6384, which leans towards no adjustment if a required metric is absent in a font.

I think we need a more coherent and consistent resolution here, not just for ic-height. As the remaining discussion is handling the case where a required font metric is missing, which is fundamentally the same as #6384, let's close this ticket and continue our conversation in #6384.

shivamidow added a commit to shivamidow/chromium that referenced this issue Feb 8, 2024
This CL adds the ic-height metric support to font-size-adjust [1]. The
ic-height metric lets developers adjust font size relative to the height
of the CJK water glyph. A note-worthy is the fallback case where the
used font does not provide vertical font info. Such vertical font info
is often missing, but the current specification does not describe the
fallback behavior. As a temporary measure, we use the advance width of
the CJK water glyph instead until the standard fallback behavior is
established [2] if we cannot get its advance height from the font. That
would be a sensible approximation since the CJK water glyph has the same
width and height in most fonts.

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792

Test:
animations/responsive/interpolation/font-size-adjust-responsive.html
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2024
This CL adds the ic-height metric support to font-size-adjust [1]. The
ic-height metric lets developers adjust font size relative to the height
of the CJK water glyph. A note-worthy is the fallback case where the
used font does not provide vertical font info. Such vertical font info
is often missing, but the current specification does not describe the
fallback behavior. As a temporary measure, we use the advance width of
the CJK water glyph instead until the standard fallback behavior is
established [2] if we cannot get its advance height from the font. That
would be a sensible approximation since the CJK water glyph has the same
width and height in most fonts.

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792

Test:
animations/responsive/interpolation/font-size-adjust-responsive.html
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2024
This change introduces support for the ic-height metric in
font-size-adjust [1], allowing developers to adjust font size based on
the height of the CJK water glyph. Notably, in cases where the chosen
font lacks the CJK water glyph or vertical font information, there is no
specified fallback behavior in the current specification. We adopt the
approach of combining ascent and descent for fallback, similar to
Gecko's method [2], until a standard fallback behavior is defined [3].

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792 (comment)
[3] w3c/csswg-drafts#6384

Test:
animations/responsive/interpolation/font-size-adjust-responsive.html
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 16, 2024
This change introduces support for the ic-height metric in
font-size-adjust [1], allowing developers to adjust font size based on
the height of the CJK water glyph. Notably, in cases where the chosen
font lacks the CJK water glyph or vertical font information, there is no
specified fallback behavior in the current specification. We adopt the
approach of combining ascent and descent for fallback, similar to
Gecko's method [2], until a standard fallback behavior is defined [3].

[1] https://www.w3.org/TR/css-fonts-5/#valdef-font-size-adjust-ic-height
[2] w3c/csswg-drafts#8792 (comment)
[3] w3c/csswg-drafts#6384

Test:
animations/responsive/interpolation/font-size-adjust-responsive.html
external/wpt/css/css-fonts/animations/font-size-adjust-composition.html
external/wpt/css/css-fonts/font-size-adjust-ic-height.html (new)
external/wpt/css/css-fonts/parsing/font-size-adjust-computed.html
external/wpt/css/css-fonts/parsing/font-size-adjust-valid.html

Bug: 1219875
Change-Id: Ie90a6241cb7e8a55fcaf65df5a4edc1058028ebe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants