Unverified Commit 44e7206a authored by Christopher Fujino's avatar Christopher Fujino Committed by GitHub

[flutter_tools] never tree shake 0x20 (space) font codepoints on web (#128302)

Always pass the space code point (`0x20`) to the font subsetter when targetting web because the web engine relies on that character to calculate a font's height.

Fixes #127270
parent 73e1f234
......@@ -78,6 +78,7 @@ Future<Depfile> copyAssets(
logger: environment.logger,
fileSystem: environment.fileSystem,
artifacts: environment.artifacts,
targetPlatform: targetPlatform,
);
final ShaderCompiler shaderCompiler = ShaderCompiler(
processManager: environment.processManager,
......
......@@ -42,11 +42,13 @@ class IconTreeShaker {
required Logger logger,
required FileSystem fileSystem,
required Artifacts artifacts,
required TargetPlatform targetPlatform,
}) : _processManager = processManager,
_logger = logger,
_fs = fileSystem,
_artifacts = artifacts,
_fontManifest = fontManifest?.string {
_fontManifest = fontManifest?.string,
_targetPlatform = targetPlatform {
if (_environment.defines[kIconTreeShakerFlag] == 'true' &&
_environment.defines[kBuildMode] == 'debug') {
logger.printError('Font subsetting is not supported in debug mode. The '
......@@ -82,6 +84,7 @@ class IconTreeShaker {
final Logger _logger;
final FileSystem _fs;
final Artifacts _artifacts;
final TargetPlatform _targetPlatform;
/// Whether font subsetting should be used for this [Environment].
bool get enabled => _fontManifest != null
......@@ -129,11 +132,17 @@ class IconTreeShaker {
}
final Map<String, _IconTreeShakerData> result = <String, _IconTreeShakerData>{};
const int kSpacePoint = 32;
for (final MapEntry<String, String> entry in fonts.entries) {
final List<int>? codePoints = iconData[entry.key];
if (codePoints == null) {
throw IconTreeShakerException._('Expected to font code points for ${entry.key}, but none were found.');
}
if (_targetPlatform == TargetPlatform.web_javascript) {
if (!codePoints.contains(kSpacePoint)) {
codePoints.add(kSpacePoint);
}
}
result[entry.value] = _IconTreeShakerData(
family: entry.key,
relativePath: entry.value,
......@@ -155,7 +164,6 @@ class IconTreeShaker {
required String outputPath,
required String relativePath,
}) async {
if (!enabled) {
return false;
}
......
......@@ -21,6 +21,8 @@ const String inputPath = '/input/fonts/MaterialIcons-Regular.otf';
const String outputPath = '/output/fonts/MaterialIcons-Regular.otf';
const String relativePath = 'fonts/MaterialIcons-Regular.otf';
final RegExp whitespace = RegExp(r'\s+');
void main() {
late BufferLogger logger;
late MemoryFileSystem fileSystem;
......@@ -122,6 +124,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
expect(
......@@ -153,6 +156,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
expect(
......@@ -176,6 +180,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
expect(
......@@ -199,6 +204,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
expect(
......@@ -227,6 +233,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
final CompleterIOSink stdinSink = CompleterIOSink();
addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
......@@ -276,6 +283,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
final CompleterIOSink stdinSink = CompleterIOSink();
......@@ -308,6 +316,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
final CompleterIOSink stdinSink = CompleterIOSink();
......@@ -341,6 +350,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: platform,
);
addConstFinderInvocation(appDill.path, stdout: constFinderResultWithInvalid);
......@@ -360,6 +370,97 @@ void main() {
expect(processManager, hasNoRemainingExpectations);
});
}
testWithoutContext('Does not add 0x32 for non-web builds', () async {
final Environment environment = createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android_arm64,
);
addConstFinderInvocation(
appDill.path,
// Does not contain space char
stdout: validConstFinderResult,
);
final CompleterIOSink stdinSink = CompleterIOSink();
resetFontSubsetInvocation(stdinSink: stdinSink);
expect(processManager.hasRemainingExpectations, isTrue);
final File inputFont = fileSystem.file(inputPath)
..writeAsBytesSync(List<int>.filled(2500, 0));
fileSystem.file(outputPath)
..createSync(recursive: true)
..writeAsBytesSync(List<int>.filled(1200, 0));
final bool result = await iconTreeShaker.subsetFont(
input: inputFont,
outputPath: outputPath,
relativePath: relativePath,
);
expect(result, isTrue);
final List<String> codePoints = stdinSink.getAndClear().trim().split(whitespace);
expect(codePoints, isNot(contains('32')));
expect(processManager, hasNoRemainingExpectations);
});
testWithoutContext('Ensures 0x32 is included for web builds', () async {
final Environment environment = createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
kBuildMode: 'release',
});
final File appDill = environment.buildDir.childFile('app.dill')
..createSync(recursive: true);
final IconTreeShaker iconTreeShaker = IconTreeShaker(
environment,
fontManifestContent,
logger: logger,
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.web_javascript,
);
addConstFinderInvocation(
appDill.path,
// Does not contain space char
stdout: validConstFinderResult,
);
final CompleterIOSink stdinSink = CompleterIOSink();
resetFontSubsetInvocation(stdinSink: stdinSink);
expect(processManager.hasRemainingExpectations, isTrue);
final File inputFont = fileSystem.file(inputPath)
..writeAsBytesSync(List<int>.filled(2500, 0));
fileSystem.file(outputPath)
..createSync(recursive: true)
..writeAsBytesSync(List<int>.filled(1200, 0));
final bool result = await iconTreeShaker.subsetFont(
input: inputFont,
outputPath: outputPath,
relativePath: relativePath,
);
expect(result, isTrue);
final List<String> codePoints = stdinSink.getAndClear().trim().split(whitespace);
expect(codePoints, containsAllInOrder(const <String>['59470', '32']));
expect(processManager, hasNoRemainingExpectations);
});
testWithoutContext('Non-zero font-subset exit code', () async {
final Environment environment = createEnvironment(<String, String>{
kIconTreeShakerFlag: 'true',
......@@ -376,6 +477,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
final CompleterIOSink stdinSink = CompleterIOSink();
......@@ -408,6 +510,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
final CompleterIOSink stdinSink = CompleterIOSink(throwOnAdd: true);
......@@ -442,6 +545,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
addConstFinderInvocation(appDill.path, stdout: validConstFinderResult);
......@@ -474,6 +578,7 @@ void main() {
processManager: processManager,
fileSystem: fileSystem,
artifacts: artifacts,
targetPlatform: TargetPlatform.android,
);
addConstFinderInvocation(appDill.path, exitCode: -1);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment