[Fonts] Serialize font-family as space-separated idents when possible [chromium/src : main]
Gaurav Kumar (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Dileep Maurya
- Divyansh Mangal
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
Per the CSSWG resolution on w3c/csswg-drafts#5846, font-familynit: this `w3c/csswg-drafts#5846` by itself doesnt expand like crbug.com/<bug-id> so it seems not useful to have it here.
I would suggest doing this:
```
Per the CSSWG resolution on [1], font-family
..... rest of your description .....
[1] https://github.com/w3c/csswg-drafts/issues/5846#issuecomment-3885672626
```
The root cause was that IsInvalidFontFamily() called
IsCSSTokenizerIdentifier(), which validates a single CSS ident token.
Multi-word names contain spaces, which fail the single-ident check,
causing all multi-word names to be quoted.
This CL adds IsCSSTokenizerIdentSequence() which validates a sequence of
one or more space-separated CSS idents. When the
CSSFontFamilySerialization flag is enabled, IsInvalidFontFamily() uses
this new function instead.nit: As per chromium guidelines, use (`) for functions names, to avoid confusion with normal jargon
// <if expr="is_linux">
'DejaVu Sans';
// </if>
// <if expr="is_macosx">
'system-ui';
// </if>
// <if expr="is_win">
'Segoe UI';
// </if>
// <if expr="is_chromeos">
'Roboto';
// </if>nit: formatting error? earlier it was aligned now it is not atleast
// per the CSSWG resolution on w3c/csswg-drafts#5846.nit: same this w3c/csswg-drafts#5846 by itself doesnt expand like crbug.com/<bug-id> so better to have the link itself.
// per CSSWG resolution on w3c/csswg-drafts#5846.nit: same this w3c/csswg-drafts#5846 by itself doesnt expand like crbug.com/<bug-id> so better to have the link itself.
Related details
- Dileep Maurya
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Also the resolution has a bug if: `if that's web compatible`, so I hope we are aware of the compatibility impact of this change, good that we have the feature flag though in case of any emergency.
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
'Non-Generic Example Family Name'This WPT currently passes for all browsers, `https://wpt.fyi/results/css/css-fonts/parsing/font-computed.html?label=master&label=experimental&aligned`
will changing it cause other/certain browsers to fail?
Gaurav Kumar (Gerrit)
Gaurav Kumar added 6 comments![]( "Open in Gerrit")
Per the CSSWG resolution on w3c/csswg-drafts#5846, font-familynit: this `w3c/csswg-drafts#5846` by itself doesnt expand like crbug.com/<bug-id> so it seems not useful to have it here.
I would suggest doing this:
```
Per the CSSWG resolution on [1], font-family..... rest of your description .....
[1] https://github.com/w3c/csswg-drafts/issues/5846#issuecomment-3885672626
```
Acknowledged
The root cause was that IsInvalidFontFamily() called
IsCSSTokenizerIdentifier(), which validates a single CSS ident token.
Multi-word names contain spaces, which fail the single-ident check,
causing all multi-word names to be quoted.
This CL adds IsCSSTokenizerIdentSequence() which validates a sequence of
one or more space-separated CSS idents. When the
CSSFontFamilySerialization flag is enabled, IsInvalidFontFamily() uses
this new function instead.nit: As per chromium guidelines, use (`) for functions names, to avoid confusion with normal jargon
Acknowledged
// <if expr="is_linux">
'DejaVu Sans';
// </if>
// <if expr="is_macosx">
'system-ui';
// </if>
// <if expr="is_win">
'Segoe UI';
// </if>
// <if expr="is_chromeos">
'Roboto';
// </if>nit: formatting error? earlier it was aligned now it is not atleast
Acknowledged
nit: same this w3c/csswg-drafts#5846 by itself doesnt expand like crbug.com/<bug-id> so better to have the link itself.
Thanks. Updated to have the link of CSSWG resolution link itself.
nit: same this w3c/csswg-drafts#5846 by itself doesnt expand like crbug.com/<bug-id> so better to have the link itself.
Thanks. Added link to CSSWG discussion itself.
This WPT currently passes for all browsers, `https://wpt.fyi/results/css/css-fonts/parsing/font-computed.html?label=master&label=experimental&aligned`
will changing it cause other/certain browsers to fail?
Yes, it will fail as this is the new spec update. I have updated the blink to comply with the latest CSS spec.
Related details
- Dileep Maurya
- Divyansh Mangal
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 2 comments![]( "Open in Gerrit")
This generally looks good to me - the CSS identifier check function needs a review from a CSS reviewer.
How about the comment from jfkthame@ in
https://github.com/w3c/csswg-drafts/issues/5846#issuecomment-3896536031
What could such an improvement look like?
bool IsInvalidFontFamily(const AtomicString& string) {Can we potentially adjust the name here and describe "Invalid" more clearly? Invalid in what sense?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaurav Kumar (Gerrit)
Gaurav Kumar added 3 comments![]( "Open in Gerrit")
This generally looks good to me - the CSS identifier check function needs a review from a CSS reviewer.
How about the comment from jfkthame@ in
https://github.com/w3c/csswg-drafts/issues/5846#issuecomment-3896536031
What could such an improvement look like?
Regarding jfkthame's comment:
the concern is about the imprecise spec wording ("contains non-alphanumeric ASCII"), which taken literally would quote single spaces and thus multi-word names like `Gill Sans`. However, our implementation already handles this correctly — `IsCSSTokenizerIdentSequence` validates the font family name against the precise grammar ident ( ' ' ident )*, which naturally allows single-space-separated valid idents (so Gill Sans serializes unquoted) while rejecting leading/trailing/consecutive whitespace (so A B serializes quoted).
The refinement jfkthame is asking for is to the spec prose, not to implementations. Tests for all these cases are in `css_markup_test.cc` (IdentSequenceRejectsConsecutiveSpaces, IdentSequenceRejectsLeadingSpace, IdentSequenceRejectsTrailingSpace, MultiWordUnquoted, DoubleSpaceQuoted).
Apologies for late reply, I was OOF for past few days.
bool IsInvalidFontFamily(const AtomicString& string) {Can we potentially adjust the name here and describe "Invalid" more clearly? Invalid in what sense?
- Dileep Maurya
- Divyansh Mangal
- Dominik Röttsches
- Ragvesh Sharma
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |