do not wrap font family name #12801

nturgut · 2019-10-04T19:02:13Z

do not wrap font family name in webkit otherwise icons not show on safari 13 (both IOS and desktop)

Manually tested using Flutter Gallery app and other additions:

Iphone with safari 12
Iphone with safari 13
Desktop with safari 13
Firefox Desktop
Chrome Desktop
Chrome on Android

Additional fonts/icons tested:
"MdiIcons" (package:material_design_icons_flutter/material_design_icons_flutter.dart)
"Lucida" Grande

Fixes the issue: flutter/flutter#41218
Fixes the issue: flutter/flutter#41483

lib/web_ui/lib/src/engine/text/font_collection.dart

yjbanov

LGTM if LGT @hterkelsen

lib/web_ui/lib/src/engine/text/font_collection.dart

mdebbar

LGTM if LGT @hterkelsen

lib/web_ui/lib/src/engine/text/font_collection.dart

mdebbar · 2019-10-10T17:10:10Z

lib/web_ui/lib/src/engine/text/font_collection.dart

+    final String familyNameInQuotes = "'$family'";
+    // Regular expression to detect punctuations. For example font family
+    // 'Ahem!' falls into this category.
+    final RegExp punctuations = RegExp(r"[.,:;!`\/#\$\%\^&~\*=\-_(){}]");


Instead of listing all the possible punctuation symbols, is it possible to use something like this: RegExp(r"[^a-z0-9 ]", caseSensitive: false)? Which matches anything other than an alphabet or digit or whitespace.

I think we should just try to match the whole family name with a regex for CSS identifiers: https://developer.mozilla.org/en-US/docs/Web/CSS/custom-ident

so something like

RegExp(r"[a-zA-Z][a-zA-Z0-9\-_]*")

@hterkelsen
I think using one regex for all the matches will create maintance/readibility issues. The correct regex for a valid family is very long and hard to understand in the first look.

If we use:
static final RegExp cssIdentifiers = RegExp(r"[a-zA-Z][a-zA-Z0-9\-_]*");

This does not cover valid cases such as:

Gill Sans Extrabold

ahem ahem

If we add the whitespace and use:

static final RegExp cssIdentifiers = RegExp(r"[a-zA-Z][a-zA-Z0-9\-_\s]*");
this also does not recognize invalid cases such as:

Ahem 1989

Goudy Bookletter 1911

We can also add the digit starting token case using \b\d yet this will make the regex even longer.

Please let me know if you have a suggestion for simpler regex? Otherwise use 2 regex one @mdebbar 's other is the digit check:

static final RegExp notPunctuation = RegExp(r"[^a-z0-9\s]", caseSensitive: false);
static final RegExp startWithDigit = RegExp(r"\b\d");

Note (the documentation):
"Font family names must either be given quoted as strings, or unquoted as a sequence of one or more identifiers. This means that punctuation characters and digits at the start of each token must be escaped in unquoted font family names."

You could do:

static final RegExp cssIdentifiers = RegExp(r"[a-zA-Z][a-zA-Z0-9\-_]*(\s[a-zA-Z][a-zA-Z0-9\-_]*)+");

Please check the new version :)

I believe it is very readable. It has two parts but each of them are very small.

Thanks for the review!

lib/web_ui/lib/src/engine/text/font_collection.dart

harryterkelsen · 2019-10-10T18:00:58Z

lib/web_ui/lib/src/engine/text/font_collection.dart

+    final String familyNameInQuotes = "'$family'";
+    // Regular expression to detect punctuations. For example font family
+    // 'Ahem!' falls into this category.
+    final RegExp punctuations = RegExp(r"[.,:;!`\/#\$\%\^&~\*=\-_(){}]");


I think we should just try to match the whole family name with a regex for CSS identifiers: https://developer.mozilla.org/en-US/docs/Web/CSS/custom-ident

so something like

RegExp(r"[a-zA-Z][a-zA-Z0-9\-_]*")

lib/web_ui/lib/src/engine/text/font_collection.dart

lib/web_ui/test/text/font_collection_test.dart

nturgut · 2019-10-10T21:33:57Z

I'll send one more commit. Thanks for all the comments.

…fari 13 (both IOS and desktop)

…s addressed. Regexp will be addressed in the next commit.

[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b git log eed171f..66bf00b --no-merges --oneline 2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122) 2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015) 2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093) 2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066) 2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044) 2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121) 2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115) 2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117) 2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116) 2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits) 2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094) 2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114) 2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113) 2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112) 2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111) 2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110) 2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109) 2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108) 2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits) 2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits) 2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105) 2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037) 2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103) 2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806) 2019-10-11 [email protected] Update felt README (flutter/engine#13097) 2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits) 2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101) 2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100) 2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089) 2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096) 2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801) 2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098) 2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091) 2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (#41943) (flutter/engine#12987) 2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088) 2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090) 2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits) 2019-10-11 [email protected] java imports/style (flutter/engine#13082) 2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073) 2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092) 2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083) 2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080) 2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086) 2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085) ...

[email protected]:flutter/engine.git/compare/eed171ff3538...66bf00b git log eed171f..66bf00b --no-merges --oneline 2019-10-14 [email protected] refactoring chrome_installer (flutter/engine#13122) 2019-10-14 [email protected] Fire PlatformViewController FlutterView callbacks (flutter/engine#13015) 2019-10-14 [email protected] iOS Platform View: Fixed overrelease of the observer. (flutter/engine#13093) 2019-10-14 [email protected] [web] Add basic color per vertex drawVertices API support (flutter/engine#13066) 2019-10-14 [email protected] Support keyboard types on mobile browsers (flutter/engine#13044) 2019-10-14 [email protected] Change IO thread shader cache strategy (flutter/engine#13121) 2019-10-14 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _GTls... to xRgq0... (flutter/engine#13115) 2019-10-14 [email protected] Roll src/third_party/skia 3838fe3c82b4..a7e1b45d9c28 (1 commits) (flutter/engine#13117) 2019-10-14 [email protected] Roll fuchsia/sdk/core/mac-amd64 from E3s7G... to Lk7iT... (flutter/engine#13116) 2019-10-14 [email protected] Roll src/third_party/dart 5fad012d02..892fcf2c45 (15 commits) 2019-10-14 [email protected] Integrate more SkParagraph builder patches (flutter/engine#13094) 2019-10-13 [email protected] Roll fuchsia/sdk/core/mac-amd64 from T3Xkz... to E3s7G... (flutter/engine#13114) 2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from lJPDX... to _GTls... (flutter/engine#13113) 2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from W9PBe... to T3Xkz... (flutter/engine#13112) 2019-10-12 [email protected] Roll fuchsia/clang/linux-amd64 from q4DVY... to 2EAvs... (flutter/engine#13111) 2019-10-12 [email protected] Roll fuchsia/clang/mac-amd64 from zpVtV... to xTJWs... (flutter/engine#13110) 2019-10-12 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LsxeL... to lJPDX... (flutter/engine#13109) 2019-10-12 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ShVbh... to W9PBe... (flutter/engine#13108) 2019-10-12 [email protected] Roll src/third_party/dart 90ff37e011..5fad012d02 (6 commits) 2019-10-12 [email protected] Roll src/third_party/dart 42dcdf903c..90ff37e011 (7 commits) 2019-10-12 [email protected] Roll src/third_party/skia 8c6c8b0c42e2..3838fe3c82b4 (3 commits) (flutter/engine#13105) 2019-10-12 [email protected] Analyze framework Dart code in presubmit tests (flutter/engine#13037) 2019-10-12 [email protected] [dart_aot_runner] Complete the port of dart_aot_runner (flutter/engine#13103) 2019-10-11 [email protected] Move initialization into FlutterEngine (flutter/engine#12806) 2019-10-11 [email protected] Update felt README (flutter/engine#13097) 2019-10-11 [email protected] Roll src/third_party/dart 9f33e8da04..42dcdf903c (7 commits) 2019-10-11 [email protected] [dart_aot_runner] Generate vmservice aotsnapshots (flutter/engine#13101) 2019-10-11 [email protected] ColorFilter matrix docs (flutter/engine#13100) 2019-10-11 [email protected] cleanup gen_package.py (flutter/engine#13089) 2019-10-11 [email protected] [dart_aot_runner] Use the host_toolchain to build kernels (flutter/engine#13096) 2019-10-11 [email protected] do not wrap font family name (flutter/engine#12801) 2019-10-11 [email protected] Roll src/third_party/skia df640e6d14a5..8c6c8b0c42e2 (12 commits) (flutter/engine#13098) 2019-10-11 [email protected] Remove persistent cache unittest timeout (flutter/engine#13091) 2019-10-11 [email protected] Added FlutterActivity and FlutterFragment hook to cleanUpFlutterEngine() as symmetry for configureFlutterEngine(). (flutter#41943) (flutter/engine#12987) 2019-10-11 [email protected] use rest args for specifying test targets (flutter/engine#13088) 2019-10-11 [email protected] Snapshot the felt tool for faster start-up (flutter/engine#13090) 2019-10-11 [email protected] Roll src/third_party/dart 965b8cb1d8..9f33e8da04 (15 commits) 2019-10-11 [email protected] java imports/style (flutter/engine#13082) 2019-10-11 [email protected] Removed retain cycle from notification center. (flutter/engine#13073) 2019-10-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from U4IBp... to ShVbh... (flutter/engine#13092) 2019-10-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from fk6ou... to LsxeL... (flutter/engine#13083) 2019-10-11 [email protected] Roll src/third_party/skia 6c2b2bb02402..df640e6d14a5 (1 commits) (flutter/engine#13080) 2019-10-11 [email protected] Gen package output corrected (flutter/engine#13086) 2019-10-11 [email protected] Print more output when gen_package fails (flutter/engine#13085) ...

nturgut requested review from harryterkelsen and yjbanov October 4, 2019 19:02

googlebot added the cla: yes label Oct 4, 2019

nturgut added the platform-web Code specifically for the web engine label Oct 4, 2019

jonahwilliams reviewed Oct 4, 2019

View reviewed changes

lib/web_ui/lib/src/engine/text/font_collection.dart Outdated Show resolved Hide resolved

nturgut mentioned this pull request Oct 10, 2019

[web] Custom fonts don't render properly in WebKit browsers flutter/flutter#41483

Closed

yjbanov reviewed Oct 10, 2019

View reviewed changes

mdebbar approved these changes Oct 10, 2019

View reviewed changes

mdebbar reviewed Oct 10, 2019

View reviewed changes

lib/web_ui/lib/src/engine/text/font_collection.dart Outdated Show resolved Hide resolved

harryterkelsen suggested changes Oct 10, 2019

View reviewed changes

nturgut requested a review from harryterkelsen October 11, 2019 00:17

harryterkelsen approved these changes Oct 11, 2019

View reviewed changes

Nurhan Turgut added 4 commits October 11, 2019 14:12

do not wrap font family name in webkit otherwise icons not show on sa…

6ffb86c

…fari 13 (both IOS and desktop)

Changing the font loading to work in all browsers.

6c0b4f3

Documentation, renaming, gramatical/spelling error related PR comment…

09a704c

…s addressed. Regexp will be addressed in the next commit.

Changing regular expressions to look simpler. Adding more unit tests.

36f96ea

nturgut force-pushed the fix_icons branch from 2c19ba6 to 36f96ea Compare October 11, 2019 21:12

nturgut merged commit 3ad8c42 into flutter:master Oct 11, 2019

nturgut mentioned this pull request Oct 12, 2019

icons not show on ios (web) flutter/flutter#41218

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 14, 2019

3ad8c42 do not wrap font family name (flutter/engine#12801)

aa1a0b0

engine-flutter-autoroll mentioned this pull request Oct 14, 2019

Roll engine eed171ff3538..66bf00bca361 (44 commits) flutter/flutter#42695

Merged

mono0926 mentioned this pull request Oct 23, 2019

[web] Fonts: Material fonts/icons not rendering flutter/flutter#32245

Closed

do not wrap font family name #12801

do not wrap font family name #12801

Uh oh!

Conversation

nturgut commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdebbar Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

harryterkelsen Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

nturgut Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harryterkelsen Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

nturgut Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harryterkelsen Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nturgut commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nturgut commented Oct 4, 2019 •

edited

Loading

nturgut Oct 10, 2019 •

edited

Loading