Bug 236073 - ch unit fallback size doesn't match the spec
Summary: ch unit fallback size doesn't match the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-03 04:05 PST by Emilio Cobos Álvarez (:emilio)
Modified: 2022-02-04 23:54 PST (History)
13 users (show)

See Also:


Attachments
WIP (4.07 KB, patch)
2022-02-04 18:24 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2022-02-04 19:59 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez (:emilio) 2022-02-03 04:05:54 PST
See https://bug1753399.bmoattachments.org/attachment.cgi?id=9262064 and the associated bug report (link below).

1ch in that font matches 1em, however it should match 0.5em per spec. It was reported to Firefox as a lack of interop when dealing with ch units on fonts without a "0" glyph, but the spec is pretty clear, see https://bugzilla.mozilla.org/show_bug.cgi?id=1753399#c2

See also https://bugs.chromium.org/p/chromium/issues/detail?id=1293773
Comment 1 Radar WebKit Bug Importer 2022-02-04 16:16:25 PST
<rdar://problem/88513297>
Comment 2 Myles C. Maxfield 2022-02-04 17:22:50 PST
We don't set it to 1em; we set it to the width of .notdef in that case.
Comment 3 Myles C. Maxfield 2022-02-04 18:24:49 PST
Created attachment 450959 [details]
WIP
Comment 4 Myles C. Maxfield 2022-02-04 18:25:27 PST
Comment on attachment 450959 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=450959&action=review

> Source/WebCore/platform/graphics/Font.cpp:154
> -    m_fontMetrics.setZeroWidth(widthForGlyph(zeroGlyph));
> +    m_fontMetrics.setZeroWidth(zeroGlyph ? widthForGlyph(zeroGlyph) : platformData().size() / 2);

I guess this isn't a great fix because it isn't ch-specific.
Comment 5 Myles C. Maxfield 2022-02-04 19:59:15 PST
Created attachment 450968 [details]
Patch
Comment 6 Cameron McCormack (:heycam) 2022-02-04 21:58:00 PST
Comment on attachment 450968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450968&action=review

> LayoutTests/ChangeLog:9
> +        This can't be a WPT test because it uses an SVG font which we're the only browser to support.

Is there any reason you can't make this test use a non-SVG font?

Also I have a vague memory of writing such a WPT at one point but I might be mistaken.
Comment 7 Myles C. Maxfield 2022-02-04 22:10:17 PST
https://phabricator.services.mozilla.com/D61190 is relevant
Comment 8 Myles C. Maxfield 2022-02-04 22:16:09 PST
I think Jonathan Kew is right - WebKit's behavior isn't great here, because we won't search fallback fonts to find the font used to render the '0' character. This patch at least gets us closer to the spec, but we're probably far enough away that this test probably shouldn't be a WPT test. When I align our behavior with Firefox's, this test will end up being deleted and replaced with another one.
Comment 9 EWS 2022-02-04 22:39:45 PST
Committed r289151 (246847@main): <https://commits.webkit.org/246847@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450968 [details].
Comment 10 Emilio Cobos Álvarez (:emilio) 2022-02-04 23:54:37 PST
Thanks for fixing so fast! Fwiw there are tests for this in wpt (css/css-values/ch-unit-013-018.html). It seems those are not in the tree though.