Skip to content

Commit 05bf8fd

Browse files
authored
fix(loader): a font filename containing a space failed to preload (#1601)
Reported downstream: a fontface asset at "data/fnt/Super Bouncer.ttf" failed with `Failed loading resource`. The parser wrapped bare paths as `url(<path>)` with NO quotes, and an unquoted CSS `url()` token may not contain whitespace, so the descriptor never parsed and `load()` rejected before any request was made: url(data/fnt/Nope.ttf) NetworkError parsed, then 404 url(data/fnt/Super Bouncer.ttf) SyntaxError never parsed url('data/fnt/Super Bouncer.ttf') NetworkError parsed, then 404 Now quoted and escaped, which also covers parentheses and commas. Only `fontface` was affected, checked rather than assumed: every other transport hands the raw string to a URL-aware API that percent-encodes it and issues a real request. `fontface` is the only parser that embeds a path in a CSS grammar. Second fix in the same function: it wrote the wrapped value back onto `data.src`, mutating the caller's manifest, which is routinely a module-level constant reused across scenes. Tests cover every round-trippable asset type twice, plain and spaced, from paired fixtures — the plain case being the control that proves the fixture sound. Mutation-verified: 5 fail against the previous parser.
1 parent 426d163 commit 05bf8fd

11 files changed

Lines changed: 237 additions & 5 deletions

File tree

packages/melonjs/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
## [20.1.1] (melonJS 2) - _unreleased_
44

55
### Fixed
6+
- **A font whose filename contained a space failed to preload.** The `fontface` parser wrapped a bare path as `url(<path>)` with no quotes, and an unquoted CSS `url()` token may not contain whitespace, so `data/fnt/Super Bouncer.ttf` produced a descriptor the browser refuses to parse: `font.load()` rejected with a `SyntaxError` before any request was made, surfacing as `Failed loading resource`. The descriptor is now quoted, which also covers parentheses and commas. Only `fontface` was affected: every other asset type hands its path to the browser, which percent-encodes it and issues a real request. The parser also no longer writes its wrapped value back onto the caller's asset descriptor, which is routinely a module-level manifest reused across scenes
67
- **A collision handler that removed an object crashed the physics step.** `Detector.collisions()` dispatches `onCollision` / `onCollisionStart` in the middle of processing a pair, then reads `objX.body.*`, which `Renderable.destroy()` sets to `undefined`. A handler calling `removeChildNow()` therefore threw out of `world.update()`, at four distinct sites depending on the spelling: removing itself, removing itself *and* returning `false` (the documented opt-out, so the likeliest form), removing the other object, and the same through `onCollisionStart`. "Remove it on pickup or on hit" is the commonest thing a collision handler does; the deferred `world.removeChild()` was always safe, `removeChildNow()` was not. `BuiltinAdapter.step` needed the same guard, since it clears `body.force` after the handlers have run
78
- **A `GLTFModel` animation callback that removed its own model crashed the frame.** `update()` fires `onended` and the completion callback, then poses the hierarchy unconditionally, writing into part meshes the callback may have destroyed. The 3D sibling of the sprite bug below, needing its own guards because `GLTFModel` re-implements the animation-callback contract rather than sharing it
89
- **`timer.updateTimers()` skipped a timer whenever another one fired.** It iterated `timers` while `clearTimer()` spliced that same array, so the entry after a fired one-shot was skipped for that tick: two `setTimeout`s due on the same frame ran only the first, the second arriving a frame late. Silent, no error

packages/melonjs/src/loader/parsers/fontface.js

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,30 @@ export function preloadFontFace(data, onload, onerror) {
1818
? globalThis.document.fonts
1919
: undefined;
2020

21-
// FontFace constructor expects src in `url(...)` or `local()` format
22-
// only wrap plain paths in url(); leave url(), local(), and data URIs as-is
23-
if (!data.src.startsWith("url(") && !data.src.startsWith("local(")) {
24-
data.src = "url(" + data.src + ")";
21+
// The FontFace constructor takes a CSS source descriptor: `url(...)` or
22+
// `local(...)`. Wrap a bare path, and leave anything already in either form
23+
// (including data URIs someone has wrapped themselves) untouched.
24+
//
25+
// QUOTED, because an unquoted CSS `url()` token may not contain whitespace.
26+
// A font whose filename has a space — "Super Bouncer.ttf", an entirely
27+
// ordinary thing to ship — produced a descriptor the browser refuses to
28+
// parse, so `load()` rejected with a SyntaxError before any request was
29+
// made and the loader reported it as a failed resource. Quoting also covers
30+
// parentheses and commas, which are equally illegal bare.
31+
//
32+
// Built into a LOCAL, not written back onto `data`: the descriptor belongs
33+
// to the caller's manifest, which is routinely a module-level constant
34+
// reused across scenes and retries.
35+
let src = data.src;
36+
if (!src.startsWith("url(") && !src.startsWith("local(")) {
37+
// escape what a CSS single-quoted string cannot carry literally
38+
const escaped = src.replace(/\\/g, "\\\\").replace(/'/g, "\\'");
39+
src = `url('${escaped}')`;
2540
}
2641

2742
if (typeof fontFaceSet !== "undefined") {
2843
// create a new font face
29-
const font = new FontFace(data.name, data.src);
44+
const font = new FontFace(data.name, src);
3045
// loading promise
3146
font.load().then(
3247
() => {
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
/**
2+
* Every preloadable asset type, with and without a space in the filename.
3+
*
4+
* A space is entirely ordinary in a shipped asset name, and it broke `fontface`
5+
* outright: that parser embeds the path in a CSS `url()` token, and an
6+
* UNQUOTED `url()` may not contain whitespace, so the descriptor never parsed
7+
* and no request was made:
8+
*
9+
* SyntaxError: The source provided ('url(data/fnt/Super Bouncer.ttf)')
10+
* could not be parsed as a value list.
11+
*
12+
* Every other transport hands the raw string to the browser, which
13+
* percent-encodes it and issues a real request. Measured, against a file that
14+
* does not exist so the outcome is purely about the transport:
15+
*
16+
* fetch() (binary/json/tmx/shader/obj/mtl) -> 404, request made
17+
* Image.src (image) -> error event
18+
* script.src (js) -> error event
19+
* video.src (video) -> error event
20+
* FontFace, unquoted -> SyntaxError, NO request
21+
*
22+
* So `fontface` was the only one affected. These tests pin the whole matrix so
23+
* a future parser that builds a URL by string concatenation cannot regress it
24+
* silently, and so the paired plain-name case proves the fixture itself is
25+
* sound rather than the test passing for the wrong reason.
26+
*/
27+
import { beforeAll, describe, expect, it } from "vitest";
28+
import { boot, loader } from "../src/index.js";
29+
import { preloadFontFace } from "../src/loader/parsers/fontface.js";
30+
31+
/** load one asset descriptor, resolving to "ok" or the failure reason */
32+
const load = (asset) => {
33+
return new Promise((resolve) => {
34+
try {
35+
loader.load(
36+
asset,
37+
() => {
38+
return resolve("ok");
39+
},
40+
(err) => {
41+
return resolve(`error: ${err?.message ?? err}`);
42+
},
43+
);
44+
} catch (e) {
45+
resolve(`threw: ${e.message}`);
46+
}
47+
});
48+
};
49+
50+
describe("asset paths containing a space", () => {
51+
beforeAll(() => {
52+
boot();
53+
});
54+
55+
// Each entry is the SAME asset twice: once plainly named, once with a
56+
// space. The plain case is the control — if it fails, the fixture or the
57+
// parser is broken and the spaced case proves nothing.
58+
const matrix = [
59+
["image", "data/img/rect.png", "data/img/rect with space.png"],
60+
["json", "data/misc/plain.json", "data/misc/name with space.json"],
61+
["binary", "data/misc/plain.bin", "data/misc/name with space.bin"],
62+
["js", "data/misc/plain.js", "data/misc/name with space.js"],
63+
];
64+
65+
// Not in the round trip, and deliberately so:
66+
//
67+
// shader, tmx, tsx, obj, mtl, gltf, aseprite — all fetched through the
68+
// same `fetchData` path as "json" and "binary" above, so the transport
69+
// under test is already covered. `shader` additionally refuses to load
70+
// without an initialized Application and a GPU renderer, which would
71+
// put a WebGL context in this spec for nothing.
72+
// audio — goes through Howler, a third-party loader with its own URL
73+
// handling; worth its own test if it ever proves to matter.
74+
// video — `video.src`, measured to issue a real request for a spaced
75+
// name exactly as Image.src does.
76+
77+
for (const [type, plainSrc, spacedSrc] of matrix) {
78+
it(`loads a "${type}" asset with a plain name`, async () => {
79+
const r = await load({ name: `plain-${type}`, type, src: plainSrc });
80+
expect(r).toBe("ok");
81+
});
82+
83+
it(`loads a "${type}" asset whose name contains a space`, async () => {
84+
const r = await load({ name: `spaced-${type}`, type, src: spacedSrc });
85+
expect(r).toBe("ok");
86+
});
87+
}
88+
89+
// `fontface` cannot round-trip here without shipping a real font binary,
90+
// and it does not need to: the bug was a PARSE failure that happened before
91+
// any request, so distinguishing SyntaxError from a network outcome is the
92+
// exact discrimination that matters.
93+
describe("fontface", () => {
94+
const outcomeFor = (src) => {
95+
return new Promise((resolve) => {
96+
preloadFontFace(
97+
{ name: `probe-${src}`, src },
98+
() => {
99+
return resolve("loaded");
100+
},
101+
(error) => {
102+
return resolve(error?.name ?? "unknown");
103+
},
104+
);
105+
});
106+
};
107+
108+
it("builds a parseable descriptor for a plain name", async () => {
109+
expect(await outcomeFor("data/fnt/Plain.ttf")).not.toBe("SyntaxError");
110+
});
111+
112+
it("builds a parseable descriptor for a name with a space", async () => {
113+
expect(await outcomeFor("data/fnt/Super Bouncer.ttf")).not.toBe(
114+
"SyntaxError",
115+
);
116+
});
117+
});
118+
});
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* The `fontface` parser builds the CSS source descriptor the `FontFace`
3+
* constructor is given, and it wrapped bare paths as `url(<path>)` with no
4+
* quotes.
5+
*
6+
* An unquoted CSS `url()` token may not contain whitespace, so a font whose
7+
* FILENAME has a space — `data/fnt/Super Bouncer.ttf`, an entirely ordinary
8+
* thing to ship — produced a descriptor the browser refuses to parse, and
9+
* `font.load()` rejected before any request was made:
10+
*
11+
* SyntaxError: The source provided ('url(data/fnt/Super Bouncer.ttf)')
12+
* could not be parsed as a value list.
13+
*
14+
* which the loader surfaced as `Failed loading resource`. Measured against the
15+
* live `FontFace` implementation, quoting is what distinguishes a parse failure
16+
* from an ordinary fetch:
17+
*
18+
* url(data/fnt/Nope.ttf) -> NetworkError (parsed, 404)
19+
* url(data/fnt/Super Bouncer.ttf) -> SyntaxError (never parsed)
20+
* url('data/fnt/Super Bouncer.ttf') -> NetworkError (parsed, 404)
21+
*/
22+
import { describe, expect, it } from "vitest";
23+
import { preloadFontFace } from "../src/loader/parsers/fontface.js";
24+
25+
/**
26+
* Run the parser and report how the browser judged the descriptor it built.
27+
* @param {string} src - the `src` field of the asset descriptor
28+
* @returns {Promise<string>} the rejection's error name, or "loaded"
29+
*/
30+
const outcomeFor = (src) => {
31+
return new Promise((resolve) => {
32+
preloadFontFace(
33+
{ name: `probe-${Math.random()}`, src },
34+
() => {
35+
return resolve("loaded");
36+
},
37+
(error) => {
38+
return resolve(error?.name ?? "unknown");
39+
},
40+
);
41+
});
42+
};
43+
44+
describe("fontface parser: the CSS source descriptor", () => {
45+
it("accepts a path containing spaces", async () => {
46+
// The file does not exist, so the honest outcome is a NETWORK error.
47+
// A SyntaxError means the descriptor never parsed and no request was
48+
// ever made, which is the bug.
49+
const outcome = await outcomeFor("data/fnt/Super Bouncer.ttf");
50+
expect(outcome).not.toBe("SyntaxError");
51+
});
52+
53+
it("still accepts an ordinary path with no spaces", async () => {
54+
const outcome = await outcomeFor("data/fnt/PlainName.ttf");
55+
expect(outcome).not.toBe("SyntaxError");
56+
});
57+
58+
it("accepts a path containing an apostrophe", async () => {
59+
// quoting introduces its own escaping hazard, so pin it
60+
const outcome = await outcomeFor("data/fnt/it's a font.ttf");
61+
expect(outcome).not.toBe("SyntaxError");
62+
});
63+
64+
it("accepts a path containing parentheses", async () => {
65+
const outcome = await outcomeFor("data/fnt/font (1).ttf");
66+
expect(outcome).not.toBe("SyntaxError");
67+
});
68+
69+
it("leaves an explicit url(...) descriptor alone", async () => {
70+
const outcome = await outcomeFor("url('data/fnt/Already Wrapped.ttf')");
71+
expect(outcome).not.toBe("SyntaxError");
72+
});
73+
74+
it("leaves a local(...) descriptor alone", async () => {
75+
// `local()` names an installed family; wrapping it in url() would break
76+
// it entirely. Absent locally, so any non-syntax outcome is fine.
77+
const outcome = await outcomeFor("local('Arial')");
78+
expect(outcome).not.toBe("SyntaxError");
79+
});
80+
81+
it("does not mutate the caller's asset descriptor", () => {
82+
// the manifest is the game's own object, frequently a module-level
83+
// constant reused across scenes and retries
84+
const asset = { name: "probe-mutate", src: "data/fnt/Super Bouncer.ttf" };
85+
preloadFontFace(
86+
asset,
87+
() => {},
88+
() => {},
89+
);
90+
expect(asset.src).toBe("data/fnt/Super Bouncer.ttf");
91+
});
92+
});
87 Bytes
Loading
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
melonJS-binary-fixture
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
globalThis.__spaceProbe2 = true;
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{ "ok": true, "n": 42 }
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
melonJS-binary-fixture
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
globalThis.__spaceProbe = true;

0 commit comments

Comments
 (0)