1796779 - CanvasRenderingContext2D's SerializeFontForCanvas is missing a space after the font weight

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[jylee]

Steps to reproduce:

const canvas = new OffscreenCanvas(100, 100);
canvas.getContext('2d').font = 'italic 500 20px Arial';
canvas.getContext('2d').font;

Actual results:

"italic 50020px Arial"

Expected results:

"italic 500 20px Arial"

My understanding is that there should be a space appended after the weight here:
https://searchfox.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#3448

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: Canvas2D' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Jamie Nicol [:jnicol]]

Makes sense to me, does that sound right Jonathan?

Comment 3

[Jonathan Kew [:jfkthame]]

Yes indeed - that's a bug. Kinda sad that it apparently wasn't picked up by any tests (unless we missed it among some other failures?).... we should see about adding some test coverage.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1796779 - Fix missing space in Canvas2d font serialization. r=#gfx-reviewers

Comment 5

[Jonathan Kew [:jfkthame]]

On testing: we can't really do an exhaustive set of WPT tests for the .font property because the serialization is not standardized, and there are substantial cross-browser differences (sadly). But for now, we can at least test that setting the font property, then reading it back and re-setting it with the serialized form results in the same used font, as determined by measuring a string. (The bug here causes failures in such a test because the weight value merges into the pixel size, resulting in huge fonts.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1796779 - Add a test for serialization of '.font' on offscreen canvas2d. r=#gfx-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f5fedc1660f
Fix missing space in Canvas2d font serialization. r=gfx-reviewers,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/321d39a49683
Add a test for serialization of '.font' on offscreen canvas2d. r=gfx-reviewers,jrmuizel

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/7f5fedc1660f
https://hg.mozilla.org/mozilla-central/rev/321d39a49683

Comment 9

[Jonathan Kew [:jfkthame]]

Given that this could break code that wants to use the (recently-shipped) offscreen-canvas support, and given the trivial/safe nature of the fix, I think we should consider uplifting it to Fx107.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 9299660 [details]
Bug 1796779 - Fix missing space in Canvas2d font serialization. r=#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Possible malfunction for code that tries to work with fonts in OffscreenCanvas.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial fix.
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9299660 [details]
Bug 1796779 - Fix missing space in Canvas2d font serialization. r=#gfx-reviewers

Approved for 107.0b4.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9299674 [details]
Bug 1796779 - Add a test for serialization of '.font' on offscreen canvas2d. r=#gfx-reviewers

Approved for 107.0b4.

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f443cac5cb5
https://hg.mozilla.org/releases/mozilla-beta/rev/38ca1462467b