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 and font metrics overrides #8967

Closed
shivamidow opened this issue Jun 15, 2023 · 8 comments
Closed

[css-fonts-5] font-size-adjust and font metrics overrides #8967

shivamidow opened this issue Jun 15, 2023 · 8 comments

Comments

@shivamidow
Copy link
Member

Related spec: https://drafts.csswg.org/css-fonts-5/#valdef-ascent-overridedescriptor-percentage

The spec document describes the percentage value for the font metrics overrides as follows.

'<percentage>'
The corresponding metric is replaced by the given percentage multiplied by the used font size.

That description sounds a bit obscure to me. What is the used font size? Does it mean the specified size by font-size or the computed size or the actual size to render (i.e., effective size) or something else?

The spec also needs to clarify how the font size should be adjusted when font metrics overrides and font-size-adjust are simultaneously applied. For instance,

@font-face {
  font-family: TestFont;
  src: serif;
  ascent-override: 100%;
  descent-override: 50%;
}

.size-adjusted {
  font: 20px TestFont;
  font-size-adjust: 2.0;
}

When I tested, Gecko applied the metric override after font-size-adjust. So, Chromium is following the Gecko's behavior [1], but I hope the spec would be improved here.

Proposed solution:
Change the wording like the following;
The corresponding metric is replaced by the given percentage multiplied by the effective font size.
or
The corresponding metric is replaced by the given percentage multiplied by the font size after adjustment.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/4305200

@drott
Copy link
Collaborator

drott commented Jul 6, 2023

Related, but I think we discussed this: Does the spec already clarify the case of how the size-adjust @font-face descriptor interacts with font-size-adjust?

PS:

When I tested, Gecko applied the metric override after font-size-adjust.

Do you mean by that, the font size is first scaled (up, 2.0x in your example), then he ascent/descent overrides are applied?

@shivamidow
Copy link
Member Author

Related, but I think we discussed this: Does the spec already clarify the case of how the size-adjust @font-face descriptor interacts with font-size-adjust?

I think so. The level 5 draft mentions

The font-size-adjust property is applied after the size-adjust descriptor.
Note: The consequence of applying font-size-adjust after size-adjust is that size-adjust appears to have no effect.

PS:

When I tested, Gecko applied the metric override after font-size-adjust.

Do you mean by that, the font size is first scaled (up, 2.0x in your example), then he ascent/descent overrides are applied?

Yes. Here is the test.

@svgeesus
Copy link
Contributor

Sorry to have ignored this issue; I try to keep on top of issues tagged css-fonts-4 but don't check the css-fonts-5 tag often enough.

Proposed solution:
Change the wording like the following;
The corresponding metric is replaced by the given percentage multiplied by the effective font size.
or
The corresponding metric is replaced by the given percentage multiplied by the font size after adjustment.

@shivamidow @drott

It sounds as if there is a desire to change the order of operations so that the spec says to apply font-size-adjust first, and then the overrides are applied? Which would align the spec to what Blink and Gecko currently do, and what the WPT is testing for?

@shivamidow
Copy link
Member Author

It sounds as if there is a desire to change the order of operations

I intend to clarify the order of operations rather than change the existing order. The current description makes it hard to figure out the order between font-size-adjust and font metric overrides.

so that the spec says to apply font-size-adjust first, and then the overrides are applied?

Yes.

Which would align the spec to what Blink and Gecko currently do,

Yes. That is aligned with the current behaviors of Gecko and Blink. (FYI, WebKit does not support font metric overrides yet).

and what the WPT is testing for?

font-size-adjust-metrics-override.html tests the behavior.
https://wpt.fyi/results/css/css-fonts/font-size-adjust-metrics-override.html?label=experimental&label=master&aligned

@shivamidow
Copy link
Member Author

I see where the confusion is coming from.

The font-size-adjust property is applied after the size-adjust descriptor.

This sounds like the size-adjust descriptor was applied first, and then the font-size-adjust property was applied. However, my proposal and the current browser's implementation sound the opposite. (i.e., font-size-adjust first, then the descriptor)

I think the spec description above is a bit misleading. When the size-adjust descriptor and the font-size-adjust property are co-used, a conflict is caused since they both define the same font scaling factor. The spec resolves the conflict by preceding font-size-adjust over the size-adjust descriptor. (i.e., font-size-adjust overrides size-adjust). Actually, that does not mean font-size-adjust is applied after size-adjust is applied.

On the other hand, font-size-adjust and metric overrides define different font scaling factors, and they can be co-used. But we need to clarify the order in which they apply. (This is what we are doing here.) The current Gecko and Blink apply font-size-adjust first and then do the font metric override descriptor to the adjusted font size.

I don't know the authors' original intention and whether we have a general ordering rule to resolve this conflict. But I think we can clarify the order in the spec, adopting the current browser implementations if we agree.

@shivamidow
Copy link
Member Author

So, what is the next step? Do we need to bring this issue to the CSS WG meeting and get it resolved there? Or can we close this issue by simply accepting a pull request if I make the edit on the spec?

@svgeesus
Copy link
Contributor

svgeesus commented Jan 5, 2024

How about you propose some specific text, as a PR, and we get it reviewed and merge if there is consensus?

@shivamidow
Copy link
Member Author

How about you propose some specific text, as a PR, and we get it reviewed and merge if there is consensus?

O.K. I will come up with a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment