glideapps / canvas-hypertxt

The fastest way to layout wrapped text on a HTML5 canvas
MIT License
102 stars 10 forks source link

Division by 0 when text has consecutive newlines #12

Open Tintef opened 1 month ago

Tintef commented 1 month ago

An easy way to reproduce is to add the following spec to the test suite:

const consecutiveNewlinesStr = `This is a quite long string 
that will need to wrap at least a 

couple times in order to 
fit on the screen. Who knows how many times?`;

...

test("consecutives newlines", () => {
    render(<canvas data-testid="canvas" />);
    ``;

    const canvas = screen.getByTestId("canvas") as HTMLCanvasElement;
    const ctx = canvas.getContext("2d", {
        alpha: false,
    });

    expect(ctx).not.toBeNull();

    if (ctx === null) {
        throw new Error("Error");
    }

    let spanned = splitMultilineText(ctx, consecutiveNewlinesStr, "12px bold", 400, false);
    expect(spanned).toEqual([
        "This is a quite long string",
        "that will need to wrap at least a",
        "",
        "couple times in order to fit on the screen. Who",
        "knows how many times?",
    ]);

    spanned = splitMultilineText(ctx, consecutiveNewlinesStr, "12px bold", 200, false);
    console.log('spanned: ', spanned);
    expect(spanned).toEqual([
        "This is a quite long",
        "string",
        "that will need to wrap at",
        "least a",
        "",
        "couple times in order to",
        "fit on the screen. Who",
        "knows how many times?",
    ]);
});

This will fail with:

  ● multi-line-layout › consecutives newlines

    expect(received).toEqual(expected) // deep equality

    - Expected  - 7
    + Received  + 3

      Array [
    -   "This is a quite long",
    -   "string",
    -   "that will need to wrap at",
    -   "least a",
    +   "This is a quite long string",
    +   "that will need to wrap at least a",
        "",
    -   "couple times in order to",
    -   "fit on the screen. Who",
    -   "knows how many times?",
    +   "couple times in order to fit on the screen. Who knows how many times?",
      ]

By taking a closer look at the failure, the text didn't wrap at all!

I was able to track down this issue to fontMetrics.size being NaN caused by doing const avg = result.width / text.length; on line 96 of multi-line.ts as two consecutives newlines cause text to be an empty string.

Also, as far as I understand, this only happens the second time because on the first one fontMetrics is undefined so value.length is used as safeLineGuess.

I see two possible solutions, but I'm not sure which one would be prefered:

  1. Early return on measureText if Number.isNaN(avg) returning result.width

    const avg = result.width / text.length;
    
    if (Number.isNaN(avg)) {
        return result.width;
    }
  2. Not calling measureText if we have an empty string, something like:

    const text = line.slice(0, Math.max(0, safeLineGuess));
    let textWidth = 0;
    if (text) {
        measureText(ctx, line.slice(0, Math.max(0, safeLineGuess)), fontStyle, hyperMode);
    }