693610 - Drawing text to a <canvas> overlaps incorrectly (Windows, probably Azure)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Smedberg]

This got found/reported to me via the aero-window-title extension: painting the following text to a canvas will overlap the text incorrectly (although .measureText still reports the correct bounds!):

【紅魔郷 ボーカル】Foreground Eclipse - Iris 

Testcase will be attached in a moment: this works correctly on non-Windows and on Windows with acceleration disabled, so I suspect Azure.

Comment 1

[Benjamin Smedberg]

Attachment: Testcase HTML

Comment 2

[Jonathan Kew [:jfkthame]]

Looks to me like it's not handling the fact that font fallback kicks in and results in the string being rendered as several separate glyph runs, because the Japanese characters aren't supported in the default font. It appears that each glyph run is being drawn at the same starting position, instead of each glyph run being drawn from the ending position of the previous one.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch, don't overlap successive glyph runs

So a simple fix is to move the initialization of advanceSum out of the glyph-run loop, so that it accumulates across multiple runs instead of resetting for each.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

Review of attachment 566258 [details] [diff] [review]:
-----------------------------------------------------------------

This looks identical to what I was thinking of, so I'm all for it. I should've known the advanceSum wasn't per run since simple glyphs don't have another way of passing X offset.

Comment 5

[Benjamin Smedberg]

Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

Can this be reftested?

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: reftest, check that Chinese and Latin runs aren't overprinted

We can at least do a != test to check that this specific bug is fixed.

A test that the rendering of multiple glyph runs within a single canvas text call is exactly _correct_, not just that it isn't _wrong_ in this particular way, seems like it would be trickier to do in a robust and portable way, as we're depending on font fallback behavior here in order to get multiple runs.

Comment 7

[Benjamin Smedberg]

Should drawing text to a canvas and having a <span> with the same text produce identical results?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Should drawing text to a canvas and having a <span> with the same text
> produce identical results?

Although in theory that sounds plausible, I'd be pretty surprised if that was reliably true, right down to the subpixel level - especially when we're rendering the canvas via Azure and the <span> via thebes/cairo.

Comment 9

[Jonathan Kew [:jfkthame]]

Pushed the patch to -inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5152a05d616

(Reftest not landed yet, awaiting Bas's review.)

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 566258 [details] [diff] [review]
patch, don't overlap successive glyph runs

Requesting approval for Aurora and Beta. This is a regression that is liable to show up any time font fallback occurs with <canvas> text strings - which can easily occur in multilingual situations, even if it's not common in simple English-only demos.

The patch is very safe - localized to the Azure canvas DrawText function, and a simple and well-understood fix - so I don't think there's any significant risk involved in taking this for FF8 and FF9.

Comment 11

[Jonathan Kew [:jfkthame]]

Pushed reftest to -inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92db762bf8d6

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/b5152a05d616
https://hg.mozilla.org/mozilla-central/rev/92db762bf8d6

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2f3b19ebe42
https://hg.mozilla.org/releases/mozilla-beta/rev/522217082f0d

Comment 14

[AndreiD[QA]]

I tested to see if the issue is still reproducible after the landing of the fix; I got the following results:
VERFIED FIXED:
 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0 (Beta)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a2) Gecko/20111019 Firefox/9.0a2 (Aurora)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111020 Firefox/10.0a1 (Nightly)

Since all the issue is fixed on all branches setting resolution to Verified and whiteboard entry to qa!