Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 3fa6084

Browse files
authored
Fixes font-subset to not drop GSUB/GPOS/GDEF tables for variable fonts where they are needed Fixes #125704 (#41592)
This PR fixes font-subset to check to see if a font is a variable font with variable font axes before additionally dropping the GSUB/GPOS/GDEF tables. These tables were being forced dropped in all cases (even though harfbuzz had been modified to always keep them). I made the change only drop the tables for variable fonts to preserve the previous behavior in the most possible cases. This PR fixes [#125704](flutter/flutter#125704). To see the bug (or verify it is fixed) in the live web examples below you must select the font variations Fill->1 and Weight->100. (See issue [#125704](flutter/flutter#125704) for more details). The issue [#125704](flutter/flutter#125704) includes examples of the font-subset being used (and breaking) the variable fonts, as well as example of the `--no-tree-shake-icons` being used where the fonts do not break. Additionally, I have created an additional [live example](https://timmaffett.github.io/material_symbols_icons_showing_tree_shake_fixed/) where this PR has been applied to font-subset and icon tree shaking is still taking place. (Example w/ icon tree-shaking using the broken font-subset for icon tree shaking is found [here](https://timmaffett.github.io/material_symbols_icons_showing_tree_shake_bug/) ). In the example build output below note that the non-variable fonts "CupertinoIcons.ttf" and "MaterialIcons-Regular.otf" have the same size savings as before the change, but the variable fonts "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf", "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf", and "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" now have a much more reasonable saving of ~2% because every icon in the font is included in the example. The previous extra ~30% savings was from having the GSUB table removed. The 30% size savings for a tree-shaking for a case where *every* icon is used in the example probably should have been suspect.. lol. Output of build using fixed font-subset.exe [live fix example](https://timmaffett.github.io/material_symbols_icons_showing_tree_shake_fixed/) : ```console flutter build web --release --web-renderer canvaskit --base-href "/material_symbols_icons_showing_tree_shake_fixed/" Font asset "CupertinoIcons.ttf" was tree-shaken, reducing it from 283452 to 1236 bytes (99.6% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialIcons-Regular.otf" was tree-shaken, reducing it from 1645184 to 10808 bytes (99.3% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 5848492 to 5683212 bytes (2.8% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 6944756 to 6779476 bytes (2.4% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 9361824 to 9196544 bytes (1.8% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Compiling lib\main.dart for the Web... 79.5s ``` BEFORE font-subset fix [live bug example here](https://timmaffett.github.io/material_symbols_icons_showing_tree_shake_bug/): ```console flutter build web --release --web-renderer canvaskit --base-href "/material_symbols_icons_showing_tree_shake_bug/" Font asset "CupertinoIcons.ttf" was tree-shaken, reducing it from 283452 to 1236 bytes (99.6% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsSharp[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 5848492 to 4079548 bytes (30.2% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsOutlined[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 6944756 to 4781576 bytes(31.1% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialIcons-Regular.otf" was tree-shaken, reducing it from 1645184 to 10808 bytes (99.3% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Font asset "MaterialSymbolsRounded[FILL,GRAD,opsz,wght].ttf" was tree-shaken, reducing it from 9361824 to 6397020 bytes (31.7% reduction). Tree-shaking can be disabled by providing the --no-tree-shake-icons flag when building your app. Compiling lib\main.dart for the Web... 63.8s ```
1 parent 58cc541 commit 3fa6084

File tree

6 files changed

+61
-8
lines changed

6 files changed

+61
-8
lines changed
4.9 KB
Binary file not shown.
7.66 KB
Binary file not shown.
10.1 KB
Binary file not shown.
55 KB
Binary file not shown.

tools/font-subset/main.cc

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,20 @@ struct HarfBuzzSubset {
5959
// The prior version of harfbuzz automatically dropped layout tables,
6060
// but in the new version they are kept by default. So re-add them to the
6161
// drop list to retain the same behaviour.
62-
63-
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
64-
HB_TAG('G', 'S', 'U', 'B'));
65-
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
66-
HB_TAG('G', 'P', 'O', 'S'));
67-
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
68-
HB_TAG('G', 'D', 'E', 'F'));
69-
62+
if (!hb_ot_var_has_data(face) || hb_ot_var_get_axis_count(face) == 0) {
63+
// we can only drop GSUB/GPOS/GDEF for non variable fonts, they may be
64+
// needed for variable fonts (guessing we need to keep all of these, but
65+
// in Material Symbols Icon variable fonts if we drop the GSUB table (they
66+
// do not have GPOS/DEF) then the Fill=1,Weight=100 variation is rendered
67+
// incorrect. (and other variations are probably less noticibly
68+
// incorrect))
69+
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
70+
HB_TAG('G', 'S', 'U', 'B'));
71+
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
72+
HB_TAG('G', 'P', 'O', 'S'));
73+
hb_set_add(hb_subset_input_set(input, HB_SUBSET_SETS_DROP_TABLE_TAG),
74+
HB_TAG('G', 'D', 'E', 'F'));
75+
}
7076
return HarfbuzzWrappers::HbFacePtr(hb_subset_or_fail(face, input));
7177
}
7278
};

tools/font-subset/test.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@
2828
SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
2929
SRC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '..', '..', '..'))
3030
MATERIAL_TTF = os.path.join(SCRIPT_DIR, 'fixtures', 'MaterialIcons-Regular.ttf')
31+
VARIABLE_MATERIAL_TTF = os.path.join(
32+
SCRIPT_DIR, 'fixtures', 'MaterialSymbols-Variable.ttf'
33+
)
3134
IS_WINDOWS = sys.platform.startswith(('cygwin', 'win'))
3235
EXE = '.exe' if IS_WINDOWS else ''
3336
BAT = '.bat' if IS_WINDOWS else ''
@@ -67,6 +70,27 @@
6770
r'0xE004',
6871
r'0xE021',
6972
]),
73+
# repeat tests with variable input font and verified variable output goldens
74+
(True, '1variable.ttf', VARIABLE_MATERIAL_TTF, [r'57347']),
75+
(True, '1variable.ttf', VARIABLE_MATERIAL_TTF, [r'0xE003']),
76+
(True, '1variable.ttf', VARIABLE_MATERIAL_TTF, [r'\uE003']),
77+
(False, '1variable.ttf', VARIABLE_MATERIAL_TTF,
78+
[r'57348']), # False because different codepoint
79+
(True, '2variable.ttf', VARIABLE_MATERIAL_TTF, [r'0xE003', r'0xE004']),
80+
(
81+
True, '2variable.ttf', VARIABLE_MATERIAL_TTF, [
82+
r'0xE003',
83+
r'0xE004',
84+
r'57347',
85+
]
86+
), # Duplicated codepoint
87+
(
88+
True, '3variable.ttf', VARIABLE_MATERIAL_TTF, [
89+
r'0xE003',
90+
r'0xE004',
91+
r'0xE021',
92+
]
93+
),
7094
)
7195

7296
FAIL_TESTS = [
@@ -95,6 +119,29 @@
95119
]), # empty input
96120
([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], []), # empty input
97121
([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], ['']), # empty input
122+
# repeat tests with variable input font
123+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [
124+
'0xFFFFFFFF',
125+
]), # Value too big.
126+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [
127+
'-1',
128+
]), # invalid value
129+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [
130+
'foo',
131+
]), # no valid values
132+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [
133+
'0xE003',
134+
'0x12',
135+
'0xE004',
136+
]), # codepoint not in font
137+
([FONT_SUBSET, 'non-existent-dir/output.ttf', VARIABLE_MATERIAL_TTF], [
138+
'0xE003',
139+
]), # dir doesn't exist
140+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [
141+
' ',
142+
]), # empty input
143+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], []), # empty input
144+
([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], ['']), # empty input
98145
]
99146

100147

0 commit comments

Comments
 (0)