Closed Bug 693610 Opened 8 years ago Closed 8 years ago
Drawing text to a <canvas> overlaps incorrectly (Windows, probably Azure)
519 bytes, text/html
1.63 KB, patch
|Details | Diff | Splinter Review|
2.03 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.
Attachment #566258 - Flags: review?(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.
Attachment #566258 - Flags: review?(bas.schouten) → review+
Comment on attachment 566258 [details] [diff] [review] patch, don't overlap successive glyph runs Can this be reftested?
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.
Attachment #566321 - Flags: review?(bas.schouten)
Should drawing text to a canvas and having a <span> with the same text produce identical results?
(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.
Pushed the patch to -inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5152a05d616 (Reftest not landed yet, awaiting Bas's review.)
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.
Attachment #566321 - Flags: review?(bas.schouten) → review+
Pushed reftest to -inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/92db762bf8d6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.